4 ms·
> We did not merge this because it gave miod@ a bad feeling. Not even sure what the right reaction to that is. On one hand, good for you for skipping an insecu
by chx 11y ago
> We did not merge this because it gave miod@ a bad feeling.
Not even sure what the right reaction to that is. On one hand, good for you for skipping an insecure patch but on the other hand, only a bad feeling? You didn't realize it's a security hole?
- zeeboo 11y agoNeither did the people writing it, or merging it. Security isn't an obvious property of code or there would be no security bugs. You have to rely on heuristics, and in this case the heuristics were right.
- chops 11y agoSurely sometimes when you glance over a patch that even without actually working through the code, you get a vibe that something doesn't seem quite right. I know I've gotten patches like this before, and my code isn't nearly as mission-critical and complicated as the SSL libraries used by basically everyone on the planet.
- jjuhl 11y agoWhen reviewing code you sometimes miss things. It's not an exact science. You also sometimes get a bad feeling without being able to say exactly what's wrong - it just feels/looks bad. That's when you should seek additional input from your peers. Rejecting code based on 'smells wrong' can be sane. The submitter should then explain better why the code is sane.