4 ms·

This is an especially weird place to complain about length bounds checking, as no user-provided data is used here, and all the sizes are static at compile time.

by aftbit 3y ago

This is an especially weird place to complain about length bounds checking, as no user-provided data is used here, and all the sizes are static at compile time. curl is putting a base64-encoded 16 byte random string (which turns into 25 ASCII bytes plus the null terminator \0) into a static 40 byte buffer. https://github.com/curl/curl/blob/1d8e8c9ad1ff3351386422535f4c066db6a9ea88/lib/ws.c#L575 https://github.com/curl/curl/blob/1d8e8c9ad1ff3351386422535f... Also, just because I'm curious... can anyone who groks C better than me explain why they're using this `keyval` local variable in the first place? Why not just set `heads[3].val = randstr` then `free()` it after the header data is processed? And why is `keyval` 40 bytes instead of 26 or 32?

tom_ 3y ago
It's presumably to reduce the number of places where you might need to call free, and then fail to do so? (This might already be happening in line 580? Though in practice perhaps that case never occurs.)
- hnarn 3y ago
  How to get your open source project peer reviewed: post an LLM bug report, post it to HN and scour the comments for people reviewing your code ;-)
- rstuart4133 3y ago
  I doubt it's to reduce the number of calls to free. After all, there must be exactly one call to free() for every call to malloc(). It looks more like a coding habit this particular programmer uses to avoid forgetting to call free(). He puts it as close to the call to malloc() as possible, which reduces the changes of forgetting. It's interesting code to look at, if you are a C programmer. As you noted, his heroic efforts to always call free fails if line 580 gets executed. It never will of course, so him adding 580 reveals a certain paranoia. Perfectly understandable in a C programmer, but for most line 578 would be enough to assuage the paranoia. Others (me, for instance) would make 578 an assert, and get rid of 580. At the 1000ft level the result is the presumably the same in both cases: the program exits after printing an error on stderr, but the error message will be different. The assert makes it plain it's an internal failure, but the current method may be mistaken for a externally triggered error. Or he could have sidestepped the overflow checks and free() by using salloc() - but maybe it doesn't exist on platforms he ports to. I would do a lot of other things differently too. I guess it just shows the way old C programmers deal with the repeated beatings inflicted by the language varies a lot.
  - tom_ 3y ago
    One call per path through the code, but potentially multiple calls lexically!
stefan_ 3y ago
At that point you can just ditch both randstr and keyval entirely and encode directly into &heads[3].val since it allocates anyway. You still have to pass in a useless randlen because it will otherwise crash, oh well, the beauty of C "output" parameters. This "copy from heap into stack variable" dance doesn't save on cleanups either since there is only one unconditional return after the encode. (But I can see how you arrive at this current code if you "laid out" your needed variables at the top and then realized later Curl_base64_encode always allocates)
blibble 3y ago
using the stack is free as the space was reserved as part of the function setup and will be torn down automatically when the function returns using the heap requires more work, can fail and requires manual cleanup

by aftbit 3y ago

tom_ 3y ago

It's presumably to reduce the number of places where you might need to call free, and then fail to do so? (This might already be happening in line 580? Though in practice perhaps that case never occurs.)

hnarn 3y ago
How to get your open source project peer reviewed: post an LLM bug report, post it to HN and scour the comments for people reviewing your code ;-)
rstuart4133 3y ago
I doubt it's to reduce the number of calls to free. After all, there must be exactly one call to free() for every call to malloc(). It looks more like a coding habit this particular programmer uses to avoid forgetting to call free(). He puts it as close to the call to malloc() as possible, which reduces the changes of forgetting. It's interesting code to look at, if you are a C programmer. As you noted, his heroic efforts to always call free fails if line 580 gets executed. It never will of course, so him adding 580 reveals a certain paranoia. Perfectly understandable in a C programmer, but for most line 578 would be enough to assuage the paranoia. Others (me, for instance) would make 578 an assert, and get rid of 580. At the 1000ft level the result is the presumably the same in both cases: the program exits after printing an error on stderr, but the error message will be different. The assert makes it plain it's an internal failure, but the current method may be mistaken for a externally triggered error. Or he could have sidestepped the overflow checks and free() by using salloc() - but maybe it doesn't exist on platforms he ports to. I would do a lot of other things differently too. I guess it just shows the way old C programmers deal with the repeated beatings inflicted by the language varies a lot.
- tom_ 3y ago
  One call per path through the code, but potentially multiple calls lexically!

stefan_ 3y ago

At that point you can just ditch both randstr and keyval entirely and encode directly into &heads[3].val since it allocates anyway. You still have to pass in a useless randlen because it will otherwise crash, oh well, the beauty of C "output" parameters. This "copy from heap into stack variable" dance doesn't save on cleanups either since there is only one unconditional return after the encode. (But I can see how you arrive at this current code if you "laid out" your needed variables at the top and then realized later Curl_base64_encode always allocates)

blibble 3y ago

using the stack is free as the space was reserved as part of the function setup and will be torn down automatically when the function returns using the heap requires more work, can fail and requires manual cleanup