8 plus ones
Shared publicly•View activity
- Sorry to hear about the reddit ignorance. There always seems to be a portion of any given population that feels criticism is evil, and therefore criticize critics.Apr 12, 2014
- Thin-skinned and grumpy? I thought those were your good points! ;-)
I'm pretty sure you could code rings around the carpers any day of the week.Apr 12, 2014
- Hi Mike!
I also thought the "leave them alone - the team is small" comment was lame. I've seen a lot of critical bugs from doing research in this area and I was amazed that they were using an untrusted value in a memory access. I don't know what else they are looking for in code reviews if it's not that.
I've also seen way too many researchers come in after the fact to show that their tool would have found some critical bug. So, in the case of the test - if it requires you to think of the problematic case to write the test, then it's a bit of a stretch as a solution because it looks like the problem is that they didn't think of it in the first case. You might be able to make a case that if they had tried to test it more they would have thought of the case to write the test.
It terms of tools for this - it seems like a simple taint analysis would have found that a tainted (unchecked) input was being used to access memory and flagged it. I suspect most commercial static analyzers would have gotten this from the code snippet I saw.Apr 12, 2014
- Hey John, good to hear from you! :-)
Point taken about people coming in after the fact with their tools. But indeed, as you suggest, my case is that had the project had a unit testing culture, they'd be making smaller changes with tests to go with them, and the likelihood of a bug like this making it past code review and unit testing becomes exponentially smaller. That, and being in the habit of unit testing would hopefully influence how they think about coding so they're even more sensitive to issues like this and would test accordingly. So it's not just that the "tool" of unit testing by itself would've fixed the problem; it's the entire mindset, culture, and set of practices around it that would've been helpful.
If nothing else, the bugfix should've had a test like mine as a regression test. That I don't consider excusable at all. If I get a wild hair, I might even see if I can get this test submitted to OpenSSL, as I think it's not half bad as a regression case.
BTW, in the interest of full disclosure, I realized a few minutes ago that I didn't call my TearDown() function anywhere. I added it to the end of ExecuteHeartbeat(), and a couple of tests in 1.0.1g began failing in -O3 -fomit-frame-pointer mode (the default). I slipped a couple memset() calls to zero out some fixture data members before freeing them, and voila, everything passes again.Apr 12, 2014
- It's interesting that the patch didn't have a test - maybe because they want the patch to be small? I thought I read that the patch came from Google, but I'm not sure...Apr 12, 2014