1.3 was the beta stream, and 1.2.8 is the most recent 1.2 release, so in those few weeks the only stable release you've missed is 1.4.0 (which may well have not been packaged at all, if the distro was waiting a few days for bugs like this to get ironed out)
Even though I told the installer to keep the default config (which it did) but over-written /etc/nginx/conf.d/default.conf The only change I had to make was port number, since I am using varnish changed my port to 8080 for nginx. Restarted nginx.
Another note: Packages provided by Ubuntu use /etc/nginx/sites-available/ and /etc/nginx/sites-available/ for storing site configs but the package directly from Nginx stores configs in /etc/nginx/conf.d/.
Not sure what the schedule looks like in general, but last night I waited ~10 minutes after the advisory watching the packages show up one by one. Still faster than any other source, of course.
Mmm, I would say it's definitely formatted beautifully. But I agree with the poster above in saying that it's error prone.
I'm a big Nginx fan, but I think it's good by just sheer force of will... if you had like 10 people working on it it would probably fall apart. Something sqlite at least has extensive automated tests, which makes me much more confident about its quality.
I understand that they're doing string comparisons (of a kind), but what are those values defined in 'uint32_t'? Is it a translation of values into memory addresses?
If you look at where they are used, then it looks like these macros do not need to compare m[1] to c1 because whenever they are used it is already known that m[1] == 'O' (hence "str3O" in the name -- that's 3 and the letter O, not number zero).
I guess c1 is included in the first case because it's faster to compare the whole 32-bit word than mask out the known byte (and since c0, c1, c2 and c3 are compile time constants, the whole right-hand-side of the comparison will be optimized by constant folding anyway).
(It's ridiculous micro-optimization in my opinion, but at least it is correct, and it is limited to a single file.)
I spend most of my time in dynamic languages so I have to ask... is this that much faster than a good regex library that it warrants a hand rolled state machine? How normal is something like this in a typical C/C++ codebase?
People (should) only implement things in C/C++ after thoroughly profiling the dynamic language POC and being sure they need the performance that a C implementation affords. So yes, the C implementation is going to sacrifice readability for performance. Of course, there are better and worse ways of accomplishing the same thing.
A "good" regex library will dynamically generate code; and if the regex is simple enough, generate code that implements a DFA rather than NFA (or PDA, for Perl regexes). So for a "good" library, no, it won't be much faster.
But very few regex libraries are that "good", because it's a combination of extreme speed with very low flexibility (DFA has exponential state explosion in worst cases compared to equivalent NFA, and isn't able to deal with e.g. backreferences). The vast majority of good regex libraries will be an order of magnitude slower. Average regex libraries included in most language distributions will be slower again.
The chief exception is compiler lexer generators like lex and flex. They produce code very similar to the state machine linked. And that's probably the most common place to see this kind of thing.
Encoding the state of the machine implicitly as the program counter, rather than an explicit state variable, often results in more readable code and is the more usual way to do it when writing it by hand. It also saves a register, important on some architectures. But the technique has slightly more limited expressiveness owing to needing to stick with structured programming constructs.
I haven't given this more than a cursory look, but I love how the body message for the commit that seemingly introduced this flaw is "No functional changes.": https://github.com/git-mirror/nginx/commit/8a3f6ce
Maybe I'm getting it wrong, but this comment seems so arrogant.. and with ngninx, that has one of the cleanest code bases of open source software actually used by many people :s
It doesn't look arrogant to me. And it doesn't matter how clean the code looks like if it uses trillions of mutable states, lots of inconsistent interfaces, occasional pointer arithmetics, manual memory management, gotos all over the place and no module tests whatsoever. I'd say nginx is a pretty error prone piece of software, just like most of the software written in C.
I didn't mean nginx parser and state machines in general as mutable states, but rather state changes in variables across multiple functions and files. They are impossible to keep track of and nginx is full of them.
To me "no functional changes" just means that the commit does not mean to introduce/change any functionality, for example if you refactor code or clean up.
* Has functional change + introduces bugs
* No functional changes + introduces bugs
* Has functional changes + doesn't introduce bugs
* No functional changes + doesn't introduce bugs
Also I haven't seen too many commit that say "this introduces 5 new accidental bugs into the system". Just saying, not sure what ou are criticizing here
GOTO is often involved in error handling. If there are a bunch of different ways that code inside a function can fail, and the cleanup after failure involves tearing down the same objects, etc... then it's usually considered acceptable to use GOTO to target it.
It's not uncommon in C as an alternative to exceptions. You don't want to be using it for every-day flow control, but when you're checking results from syscalls repeatedly, using a gigantic nested if-else chain would get more messy than using goto. The other alternative is early return/exit calls, but then you have to remember to clean up everything each time before you return/exit.
In this case goto is used as a form of "exception" handling, which is a perfectly fine use case for goto if you have an imperative language like C without exceptions or exception handling mechanisms. In fact it would probably be bad style not to use goto in this case.
If you have a better way to express the control flow you should use it instead though.
55 comments
[ 3.2 ms ] story [ 120 ms ] threadMeaning, the vulnerable versions were likely not out long.
I am on Ubuntu 12.04.2 LTS, I had use "add-apt-repository ppa:nginx/development" to get nginx/1.3.12, how do I get in to the latest release?
Wiki says:
Stable release 1.4.0 / 24 April 2013
Preview release 1.3.16 / 16 April 2013
How do I get into the latest stable bandwagon?
[1] http://wiki.nginx.org/Install#Official_Debian.2FUbuntu_packa...
It seems I'm stuck with 1.2.6 for the time being.
I had to install this public key from ubuntu server:
sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys ABF5BD827BD9BF62
Had to remove nginx-full and nginx-common.
Then install nginx: apt-get install nginx
Even though I told the installer to keep the default config (which it did) but over-written /etc/nginx/conf.d/default.conf The only change I had to make was port number, since I am using varnish changed my port to 8080 for nginx. Restarted nginx.
Thanks for the help!I just wish it had a make test
Ubuntu 12.04.02 LTS uses Version: 1.1.19-1.
I'm a big Nginx fan, but I think it's good by just sheer force of will... if you had like 10 people working on it it would probably fall apart. Something sqlite at least has extensive automated tests, which makes me much more confident about its quality.
I guess c1 is included in the first case because it's faster to compare the whole 32-bit word than mask out the known byte (and since c0, c1, c2 and c3 are compile time constants, the whole right-hand-side of the comparison will be optimized by constant folding anyway).
(It's ridiculous micro-optimization in my opinion, but at least it is correct, and it is limited to a single file.)
In the first case, comparing 2 integer (4x 8bit) is faster than omitting one byte.
That's is, using ngx_str3Ocmp with c1 != 'O' would be a usage error.
Edit: sltkr, sorry didn't see your reply.
But very few regex libraries are that "good", because it's a combination of extreme speed with very low flexibility (DFA has exponential state explosion in worst cases compared to equivalent NFA, and isn't able to deal with e.g. backreferences). The vast majority of good regex libraries will be an order of magnitude slower. Average regex libraries included in most language distributions will be slower again.
The chief exception is compiler lexer generators like lex and flex. They produce code very similar to the state machine linked. And that's probably the most common place to see this kind of thing.
Encoding the state of the machine implicitly as the program counter, rather than an explicit state variable, often results in more readable code and is the more usual way to do it when writing it by hand. It also saves a register, important on some architectures. But the technique has slightly more limited expressiveness owing to needing to stick with structured programming constructs.
Found with:
Also, are you sure this change introduces the flaw? It looks like it could have been there before the refactor, as well.
https://github.com/git-mirror/nginx/commit/7c7ad803a13d5f73b...
That's a pretty big statement right there.
You are just saying that C is not clean. (IMHO) nginx code-base is doing a really good job dealing with micro-optimization.
>code looks like if it uses trillions of mutable state
Actually, that's a very good approach. e.g. : nginx parser get optimized out as a big jump table (http://en.wikipedia.org/wiki/Branch_table)
If you have a better way to express the control flow you should use it instead though.
If you are an expert programmer, you can use it in some parts where efficiency is important. Finite State Machine implementations are a common use.
It's pointless to check for such a signed integer overflow after it happened.