Can anyone explain this to me? Is he saying that one of the fixes is better than the other? Or is he saying that both of them are good and both are better than the code they are fixing?
I think he’s saying the second one, but I’m not sure.
Well, tedu hangs out here, so he can explain himself. But, here’s my interpretation:
Both versions fix the common bug of not checking return values. In this case, ssl3_handshake_mac can return 0, and if that’s the case, then this function (ssl3_final_finish_mac) should stop and return 0.
The first diff is better. Specifically, because it “eliminates semantic overloading of variables.” That is, in the first diff, there are only two variables: ret_md5 and ret_sha1. Unsurprisingly, these are respectively the return values for the md5 and sha1 ssl3_handshake_mac calls. The important part is when I typed “unsurprisingly.” The variables are used for one thing and assigned once (never change). They’re very easy to understand.
Conversely, in the second diff, there is ret and sha1len. sha1len is the same as ret_sha1 in the first diff. ret, however, is both ret_md5 and then later equivalent to (ret_md5 + ret_sha1). You have the read the whole function through to understand what it is at any point because it will have two different values. The meaning of ret is semantically overloaded.
(ret_md5 + ret_sha1)
Thanks for that, that makes it a lot clearer.
As quad said.
Although this function isn’t likely to go through much more development, in the second case it’s possible for someone to accidentally insert a “return ret” statement into the middle of it, reintroducing the bug. Eliminate that variable and it will be more obvious in code review that returning “ret_md5” is a bug.
I haven’t used C in anger for a long time.
As as I recall, accumulative variables are a common pattern in a lot of classic C code. Certainly it’s come up a few times in the libressl cleanup. But, I’d never really considered the danger of a misevaluated escape creeping in.
tedu, thanks for sharing your insight, both here and on your blog.
My kneejerk reaction when reviewing diffs is to want code structured in a way where discrete functionality changes are grouped closed to each other. The idea being, if it has to be backed out, it happens in one place rather than all over. But, this example makes a persuasive argument for situations where that rule results in more fragile code. Food for thought!
Certainly, I agree minimal fixes are preferable to the CADT model but anything can be taken too far. One of the problems with OpenSSL is that it is the result of many, many years of the accretion of minimal fixes, without anyone asking if maybe the bug being fixed was representative of a larger class of bugs; or if changes could be made to prevent such bugs in the future.
Styles and fashions change over time. There is a “unix greybeard” style of programming that goes like:
errno = 0
if (errno) printf("oops\n");
That’s not my preferred style, and I think it’s interesting to see that Go now effectively forbids that style of programming, making you handle each error as it occurs.