> This check for headers_array in the loop’s termination condition is only in the upstream code, not in the one that was in Ubuntu at the time I reported the bug and submitted the patch.
Yes. That's why I'm pointing out the difference and suggesting that we reconcile the code.
> When I made the patch I decided to put a proper if-clause there because it’s easier to understand what is going on (the check in the for loop seems like obfuscated programming to me)
Agreed. Happy to accept a patch which cleans that up in the upstream tree. Although...
> and technically it’s even faster to check only once instead of at every loop iteration (even though this minimal performance gain is hardly significant here).
In practice any compiler worth its salt should optimise that away because it'll know that calling free() won't affect the value of headers_array. See in 'objdump -S' output how the loop only jump back from 0x8d98 to 0x9d88, adding 8 to the pointer every time until it reaches the precalculated end value.
free(cookie_array);
8d6a: 48 8b 3c 24 mov (%rsp),%rdi
8d6e: e8 ed da ff ff call 6860 <free@plt>
for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
8d73: 4d 85 ff test %r15,%r15
8d76: 74 22 je 8d9a <cookie_cb+0x25a>
8d78: 43 8d 44 24 01 lea 0x1(%r12,%r12,1),%eax
8d7d: 4c 89 fd mov %r15,%rbp
8d80: 4d 8d 64 c7 08 lea 0x8(%r15,%rax,8),%r12
8d85: 0f 1f 00 nopl (%rax) free(headers_array[i]);
8d88: 48 8b 7d 00 mov 0x0(%rbp),%rdi
for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
8d8c: 48 83 c5 08 add $0x8,%rbp free(headers_array[i]);
8d90: e8 cb da ff ff call 6860 <free@plt>
for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
8d95: 49 39 ec cmp %rbp,%r12
8d98: 75 ee jne 8d88 <cookie_cb+0x248> free(headers_array);
8d9a: 4c 89 ff mov %r15,%rdi
8d9d: e8 be da ff ff call 6860 <free@plt>
> This check for headers_array in the loop’s termination condition is only in the upstream code, not in the one that was in Ubuntu at the time I reported the bug and submitted the patch.
Yes. That's why I'm pointing out the difference and suggesting that we reconcile the code.
> When I made the patch I decided to put a proper if-clause there because it’s easier to understand what is going on (the check in the for loop seems like obfuscated programming to me)
Agreed. Happy to accept a patch which cleans that up in the upstream tree. Although...
> and technically it’s even faster to check only once instead of at every loop iteration (even though this minimal performance gain is hardly significant here).
In practice any compiler worth its salt should optimise that away because it'll know that calling free() won't affect the value of headers_array. See in 'objdump -S' output how the loop only jump back from 0x8d98 to 0x9d88, adding 8 to the pointer every time until it reaches the precalculated end value.
8d6a: 48 8b 3c 24 mov (%rsp),%rdi
8d6e: e8 ed da ff ff call 6860 <free@plt>
for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
8d73: 4d 85 ff test %r15,%r15
8d76: 74 22 je 8d9a <cookie_cb+0x25a>
8d78: 43 8d 44 24 01 lea 0x1(%r12,
8d7d: 4c 89 fd mov %r15,%rbp
8d80: 4d 8d 64 c7 08 lea 0x8(%r15,
8d85: 0f 1f 00 nopl (%rax)
8d88: 48 8b 7d 00 mov 0x0(%rbp),%rdi
for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
8d8c: 48 83 c5 08 add $0x8,%rbp
8d90: e8 cb da ff ff call 6860 <free@plt>
for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
8d95: 49 39 ec cmp %rbp,%r12
8d98: 75 ee jne 8d88 <cookie_cb+0x248>
8d9a: 4c 89 ff mov %r15,%rdi
8d9d: e8 be da ff ff call 6860 <free@plt>