Hacker News Reader

Comment by ctz | Hacker News Reader

This patch is setting OPENSSL_NO_HEARTBEAT, but openssl actually only disables heartbeats when you set OPENSSL_NO_HEARTBEATS (note trailing S).

Someone should make a thing which does sentiment analysis on commit messages, and flags vitriolic or angry commits as possibly containing more typos than usual.

View on HN

That's almost too funny.

Edit: Does that last "ok" line mean that two other committers reviewed and OK'ed this patch as well? Three openbsd committers approved a one-line change that didn't fix a single thing? (And people wonder how the OpenSSL bug could live on for two years =) )

The typo was fixed a little while later:

http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libssl/ssl/Mak...

It is but we should have a dose of humility in mocking them since many, many people [including everyone on this site that is a dev] make stupid mistakes from time to time. Such is the nature of the profession. None of us commit perfect code the first time, every time.

Exactly.

I'll just leave this here. http://article.gmane.org/gmane.os.openbsd.misc/211963

Wha-what? That's pretty crazy. At the very least, if OpenSSL goes through this trouble, why doesn't it just implement its own malloc and friends that are both fast and secure? In fact, shouldn't it include something like scurb_and_free() so that sensitive bits would actually be erased when not needed. That seems like a much better solution than the approach described in this link.

It gets worse, see http://www.tedunangst.com/flak/post/analysis-of-openssl-free...

Essentially the code mistakenly relies on the freelist being LIFO and not being scrubbed.

Whoever said that OpenSSL was written by monkeys continues to be proven right.

That's a fairly rude thing to say. Would you say that to their faces? This is a pretty large site, and a lot of people stop by here, possibly including some of the "monkeys" .

By all means, call the code what you want, but separate that from the people.

something tells me a similar snafu made by a CS student would get "fairly rude" comments in reply by any competent instructor or advisor.

you say that and yet I have had to teach people coming from several fairly top institutions that "no your lecturer was wrong, it is inappropriate to directly memcpy() from the network into a struct"

Well Ted Nugent (not to mention Ann Coulter) got away with calling Obama a "monkey," and he's the highly respected darling of the GOP, whose endorsement is sought after and appreciated by not only Republican senators and congressmen, but Mitt Romney himself.

(I'm not endorsing anyone's behavior here, just pointing out how low the right hand side of modern American politics has degenerated, to provide some perspective.)

The really odd bit is that all the buffer management operations in OpenSSL memset to 0, even when it appears not entirely necessary.

However, the freelist code somehow fails to follow this convention.

Many projects that care about performance and memory fragmentation use per-purpose free lists, and not malloc/free.

The performance is significantly better, and there is a much smaller danger of running out of space due to fragmentation issues.

Good advice for for the author of that rage-filled patch as well.

Well, he has good reason to be enraged. The bug is a horrendous one and on his operating system was due to the developers deliberately the operating system provided security protections. Meanwhile his slightly buggy one liner failed in such a way that it did nothing, before being fixed 3 hours later.

Absolutely.

What matters is not to always be right, but how you correct errors.

"What matters is not to always be right, but how you correct errors."

In my opinion, heartbleed is a counterexample to your point. It's a case where making the mistake at all caused a lot of damage, no matter how quickly they patched it.

Well, it can be argued that this instance of mistake happened BECAUSE they didn't fix their review process the last time a silly bug managed to get in.

So there's enough blame and rage to go around. Any leader with a spine has to first contemplate "how did I fuck up?"

What if your error was running someone over with your car?

Yes and no. The major mistake is not a missing "S". The major mistake is not performing a test which would show that the change was made (or in this case that it wasn't made).

Test our changes before we commit? What blasphemy is this?

At the very least the organization should look at itself very carefully, find out how this this happened.

Hindsight is 20/20.

So, basically, the patch wasn't tested. This whole thing is getting more and more embarrassing.

This patch is setting OPENSSL_NO_HEARTBEAT, but openssl actually only disables heartbeats when you set OPENSSL_NO_HEARTBEATS (note trailing S).

Having negative ifdefs in the first place is a pretty bad practice. This clearly shows why.

If all your IFDEFs are "positive" they have to be declared somewhere and then it's easy to "deactivate" the things you no longer want, by simply commenting a line out.

It reduces the possibility for error, and gives you a better picture of how many IFDEF conditions you are dealing with around your code.

A security bug may well require enabling a feature, rather than disabling one, and then you end up with exactly the same problem.

I personally detest #ifdef's, and would rather have multiple .c files that I can choose from to include in my build. Code infected with #ifdef'itis is unreadable, difficult to test, difficult to maintain, ...

They caught it. It is in a newer commit now. Still kind of "funny".

Congratulations, you have hereby been granted commit privileges to libssl. Have fun!

It just goes to show, paying attention has a cost, energy. When energy is expended on a fiery commit message, the message can become a distraction. There is an energetic cost to all actions, and strategies.

That being said, I fully endorse this commit. There seem to be good intentions behind it, and it was fixed soon after.

I bet big dev shops would actually pay for this to know whom needs a vacation, etc.

That will just drive people to make sarcastic commit messages.

Reply on news.ycombinator.com