4 ms·

Patch is pretty interesting. Why was buffer overflow protection behind a debug flag? http://nginx.org/download/patch.2014.spdy2.txt http://nginx.org/download/p

Patch is pretty interesting. Why was buffer overflow protection behind a debug flag? http://nginx.org/download/patch.2014.spdy2.txt http://nginx.org/download/patch.2014.spdy2.txt

nolok 13y ago
Usual reason for mistakes like this is: someone was debugging something then (partially) forgot to clean up before committing. Once that is done it's incredibly easy for these kind of bugs to go un-noticed for a long time.
- greglindahl 13y ago
  Assuming you don't have a test for it, sure. And don't fuzz everything on a regular basis.
  - deleted 13y ago
    [deleted]
  - deleted 13y ago
    [deleted]
willvarfar 13y ago
And why leave in a #if when patching too?
- untaken 13y ago
  Keeps the changes to a minimum, reducing the risk of effecting anything else. (quicker + safer)
- atom7 13y ago
  I assume the reason was to keep diff as minimal as possible. Note that it should apply to all mentioned versions without problems.
unwind 13y ago
Without context (I'm not familiar with nginx' code) the blatant subtraction of pointers was scary, too. As everyone is of course no doubt aware, subtraction of pointers is only valid in C if you can guarantee that the pointers point to the same "object" (or at most 1 character past the end of an object). I would prefer an interface using a size_t count of available space, rather than passing an end-pointer around. Again, this is based solely on reading the patch, so it might be perfectly fine. Considering nginx' reputation, I guess it's academic.
- atom7 13y ago
  It's a common practice to use two pointers when parsing some stream data buffer. If you will use a pointer to the start of the buffer and the size of an available data to parse, then you would need to change two variables instead of one on every parsing stage. So you have more opportunity to make a mistake.
- ambrop7 13y ago
  It's kind of implied by the parameters that they point to the same object; they're named "pos" and "end" after all. But I agree, a count variable is nicer. The subtraction also has a (very theoretical) problem that its result must fit in ptrdiff_t (signed), and it would produce undefined behavior when you have a huge object whose size doesn't fit in ptrdiff_t (but does in size_t, by definition).

Patch is pretty interesting. Why was buffer overflow protection behind a debug flag? http://nginx.org/download/patch.2014.spdy2.txt http://nginx.org/download/patch.2014.spdy2.txt

nolok 13y ago

Usual reason for mistakes like this is: someone was debugging something then (partially) forgot to clean up before committing. Once that is done it's incredibly easy for these kind of bugs to go un-noticed for a long time.

greglindahl 13y ago
Assuming you don't have a test for it, sure. And don't fuzz everything on a regular basis.
- deleted 13y ago
  [deleted]
- deleted 13y ago
  [deleted]

willvarfar 13y ago

And why leave in a #if when patching too?

untaken 13y ago
Keeps the changes to a minimum, reducing the risk of effecting anything else. (quicker + safer)
atom7 13y ago
I assume the reason was to keep diff as minimal as possible. Note that it should apply to all mentioned versions without problems.

unwind 13y ago

Without context (I'm not familiar with nginx' code) the blatant subtraction of pointers was scary, too. As everyone is of course no doubt aware, subtraction of pointers is only valid in C if you can guarantee that the pointers point to the same "object" (or at most 1 character past the end of an object). I would prefer an interface using a size_t count of available space, rather than passing an end-pointer around. Again, this is based solely on reading the patch, so it might be perfectly fine. Considering nginx' reputation, I guess it's academic.

atom7 13y ago
It's a common practice to use two pointers when parsing some stream data buffer. If you will use a pointer to the start of the buffer and the size of an available data to parse, then you would need to change two variables instead of one on every parsing stage. So you have more opportunity to make a mistake.
ambrop7 13y ago
It's kind of implied by the parameters that they point to the same object; they're named "pos" and "end" after all. But I agree, a count variable is nicer. The subtraction also has a (very theoretical) problem that its result must fit in ptrdiff_t (signed), and it would produce undefined behavior when you have a huge object whose size doesn't fit in ptrdiff_t (but does in size_t, by definition).