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.
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.
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).
The problem affects nginx 1.3.15 - 1.5.11, compiled with the
ngx_http_spdy_module module (which is not compiled by default) and
without --with-debug configure option, if the "spdy" option of the
"listen" directive is used in a configuration file.
It said it affected 1.3.15 to 1.5.11 but the fix is available in nginx 1.5.12, 1.4.7.
So for those who are using legacy version are they going to rely on distro vendor to push the patch? Just curious, even though I guess the number of users who have activated this experimental SPDY is low and people who actually have it enabled probably know how to fix it themselves.
1.5.12 is the latest devel branch and 1.4.7 is the latest release branch (released in response to this CVE and excepted from the vuln range). The patch works for any of the intervening versions.
As far as I'm aware, all the major distros backport nginx stable release
Before you panic: "The problem affects nginx 1.3.15 - 1.5.11, compiled with the
ngx_http_spdy_module module (which is not compiled by default) and
without --with-debug configure option, if the "spdy" option of the
"listen" directive is used in a configuration file."
Maybe they wanted a patch that could be easily applied to older versions. It might have an elif later too, although I'm guessing that's not too common after debug blocks.
- if (ngx_list_init(&r->headers_in.headers, r->pool, sc->entries + 3,
+ if (ngx_list_init(&r->headers_in.headers, r->pool, 20,
Personally, both the old code and the new code use magic numbers, and both send up a red flag to me. What does 20 mean? (I don't personally need an answer here: my point is it's not obvious and therefore it's easier for bugs to get through.)
31 comments
[ 3.2 ms ] story [ 75.4 ms ] thread> Thanks to Lucas Molas, researcher at Programa STIC, Fundación Dr. Manuel Sadosky, Buenos Aires, Argentina.
http://nginx.org/download/patch.2014.spdy2.txt
Once that is done it's incredibly easy for these kind of bugs to go un-noticed for a long time.
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.
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).
The problem affects nginx 1.3.15 - 1.5.11, compiled with the ngx_http_spdy_module module (which is not compiled by default) and without --with-debug configure option, if the "spdy" option of the "listen" directive is used in a configuration file.
So for those who are using legacy version are they going to rely on distro vendor to push the patch? Just curious, even though I guess the number of users who have activated this experimental SPDY is low and people who actually have it enabled probably know how to fix it themselves.
As far as I'm aware, all the major distros backport nginx stable release