
A brief history of one line fixes - coconutrandom
http://www.tedunangst.com/flak/post/a-brief-history-of-one-line-fixes
======
chimeracoder
> What do all these earlier mistakes have in common, apart from the obvious:
> being exemplars of “catastrophic loss of structural integrity”? They all
> date from before 2013. That’s how we know the NSA wasn’t involved.

I get that this is trying to make fun of the response to Apple's "goto fail;",
but this logic ("These similar errors predate 2013 → Apple's similar error was
not an NSA backdoor") seems rather faulty. There are a number of flaws with
this line of reasoning. To name a few:

* The NSA could have been involved with backdoors before 2013 (unlikely in Debian, but mentioning 2013 is a bit of a red herring)

* Apple could have been encouraged to insert a backdoor and did so in a way that gave them plausible deniability (either because the NSA specified that, or because they wanted to cover their behinds)

Whether or not this incident was the result of the NSA's prompting is
something we'll probably never know[0], but this article doesn't do much to
argue one way or the other.

[0] The only way we could know is if someone literally came out and admitted
it (or someone like Snowden were to leak it). It's possible to prove the
existence of something (ie, a backdoor attempt), but impossible to prove the
absence of something.

~~~
xenophanes
What you've argued is faulty logic was actually sarcasm. One major clue is
that the previous paragraph also contained heavy sarcasm.

~~~
joe_the_user
Uh yeah, you're right. I'm surprised that didn't jump off the page at me but
hey, I read the comments first and then the article too, failing "like a good
banker", meaning succeeding in not spotting the bullshit but not any worse
than my fellow hn-ers who also upvoted it.

I amazed your at bottom of the comments. But then again I was silly enough to
upvote the parent before I read the full text.

"A sound banker, alas, is not one who foresees danger and avoids it, but one
who, when he is ruined, is ruined in a conventional and orthodox way with his
fellows, so that no-one can really blame him" \- JM Keynes

------
fjarlq
A favorite of mine, in C:

    
    
      ASSERT(apples = 1);
      VERIFY(oranges = 2);
    

(Note the '=' means assignment, not comparison.)

Both are assertion-testers resulting in a core dump if the test fails, but
ASSERT is only defined for DEBUG builds.

Since the assignments are to nonzero (true) values, buggy values of 'apples'
are silently corrected -- but only during test runs. Buggy values of 'oranges'
are always silently corrected.

Hilarity ensued. Afterwards, 'gcc -Wparentheses' became mandatory.

~~~
fooyc
One way to avoid this kind of mistakes is to use Yoda conditions [0]:

    
    
        ASSERTS(1 == apples);
    

If you forget one `=`, it doesn't compile.

[0]:
[https://en.wikipedia.org/wiki/Yoda_conditions](https://en.wikipedia.org/wiki/Yoda_conditions)

~~~
deletes
I don't think learning this is worth it. Using this will not always warn you(
complicated expression or variables ) and you still might miss something.

+EDIT: In those occasions when you actually compare to a integer, the
expression is so simple that you immediately understand the whole line. In
case of an actual typo the compiler will warn you, and the inversion is not
needed. Nowadays, there is simply no excuse for a ignored or missed( bad
compiler ) warning.

 _Assignment in an If statement must produce a warning!_

~~~
hibbelig
Don't understand? If the constant is on the left, then forgetting one of the
equals signs will always be an error.

~~~
rguldener
Yes but I think he is saying that if the left/what you compare to is not a
constant then it will not warn you (if the type agrees, as it should anyway
for a comparison).

~~~
cbhl
If the left is a function call, it will still error out because you can't
assign to it.

If you're doing

    
    
        if (var1 == var2)
    

then yoda expressions won't save you, however.

~~~
koenigdavidmj
Also, in C++ you can assign to the return value of a function returning a
reference.

------
mindslight
_Cough_ , there's one major thing all of these bugs have in common - they're
all in C code.

This is mostly due to the volume and prominence of C. But a language with the
same fundamental semantics of C but lacking the cleverbait syntax and weakened
types would have prevented over half of these mistakes.

~~~
stusmall
I would be more willing to blame C if these bugs were unsafe memory
operations. Unused parameters, logic errors and assignment instead of
comparison aren't unique to C and the language isn't without counter measures
for them.

The X one is probably the only very C bug on the list and compiler warnings
and/or a good static code checker would have found it.

~~~
mindslight
I deliberately did not address unsafe memory operations, because the current
widely-accepted state of the art for memory safety is garbage collection,
which has significant overhead. I could have also tooted the horn for Rust (or
Felix/BitC/Deca), but my goal was not to point out blank-slate solutions, but
just how much of C itself could be trivially fixed.

~~~
stusmall
Among others the Objective-C crowd might want to have a chat with you about it
being the "widely-accepted state of the art." This is ignoring the fact that
memory management does not really address C's ability to do unsafe memory
operations as much as its just resource management.

~~~
mindslight
I really should have said garbage collection + RAII. I'm not saying that GC is
accepted by everyone, just among the people working in current memory-safe
languages. (Or are you saying that ObjC does something even better that's yet
to catch on?)

It's not so much about a language's ability to do unsafe memory operations,
but the necessity of doing them to write useful code in the language.

------
mpetrov
The commentary on last example from Tarsnap seems wrong. The error was that
the nonce stopped being incremented (see the linked Tarsnap blog post), but
the author suggests the issue there is the unbraced if. The unbraced if is
still not great style, but it wasn't the cause of the security blunder.

~~~
clarry
It is kind of on purpose, that's the whole point of the writeup. It's taking a
little jab at everyone who proclaims a "simple and obvious" way any given
error could've been avoided. Fact is, making the programmer type more (braces
or other stuff) doesn't save him from himself; the goto fail error could've
been made with braces just as well as without.

The other examples are make the same point. There are ways to help catch
specific bugs. But there is no magic "fix it" button. Consider static analysis
for example: you actually have to run it and inspect the output. You have to
interpret it right. You have to fix it right. And so on. Human error can ruin
each step, as has happened.

------
yeukhon
I have done similar thing to myself in Python, sabotaging my own little
project. The following is not a rare thing. Imagine this were initializing
random key in TLS... But since Python syntax is arguably cleaner and more
readable, it's easier to catch. But that's a bet we have to put up with...

    
    
        >>> def f(a,b=[]):
        ...     b.append(a)
        ...     return b
        ...
        >>> print f(1)
        [1]
        >>> print f(1)
        [1, 1]
        >>> print f(1)
        [1, 1, 1]
    
    
        >>> import random
        >>> def key():
        ...     return random.randint(1,10000)
        ...
    
        >>> def f2(a, b=key):
        ...     return b()
        ...
        >>> def f3(a, b=key()):
        ....    return b
        >>> f2(1)
        3974
        >>> f2(1)
        8684
        >>> f3(1)
        2867
        >>> f3(1)
        2867

~~~
GuiA
Well, any Python developer worth his salt knows that initializing default
parameters that way is a big no-no. I actually use code with those exact
mistakes in interviews, as it is a good way to tell Python beginners from more
enlightened users.

~~~
raverbashing
Correct

Unfortunately a lot of people still use it

(and it's actually correct, IF you don't modify it)

It's a risk not worth taking (to me, at least)

~~~
klibertp
That's the only way of creating function with state without introducing
closure or making an object with __call__. It's very handy for accumulators
and other bits of state which are inherent to the function you write. It's
essentially a `static` variable declared inside a function in C and believe it
or not it has it's uses.

Depending on a problem it can be worthwhile to use other techniques of
creating stateful callables, but that doesn't mean you should _never_ create a
function with mutable default argument. It's there in the language - learn
about it, understand how it works and why it works like this, then understand
where to use it and use it where it makes sense. That's a pretty generic
advice, valid for almost any language feature.

~~~
leohutson
> That's the only way of creating function with state without introducing
> closure or making an object with __call__

What about this?

    
    
      def foo():
          if not hasattr(foo, 'static_list'):
              foo.static_list = []
    

At least it's a bit easier to see that something funky is going on.

------
_pmf_
> How is this possible? Does nobody use a compiler that warns about
> comparisons always being false?

External symbols are resolved at link time; as far as the compiler is
concerned, setuid might be a symbol declared as __weak.

I don't particularly like the snide remarks towards the maintainers of these
libraries.

~~~
spc476
I ran the following code through GCC:

    
    
        #include <stdio.h>
        #include <unistd.h>
        #include <sys/types.h>
    
        int main(void)
        {
          if (getuid() != 0 && geteuid == 0)
          {
            printf("only root\n");
            return 1;
          }
          printf("okay\n");
          return 0;
        }
    

With just a plain GCC (no options), the code compiled and exhibited the bug
(it always printed "okay"). That's to be expected. What was not expected what
when I compiled the code with "gcc -ansi -Wall -Wextra -pedantic". NOT ONE
ERROR OR WARNING!

How is that happening?

Well, C allows function pointers. Also, in C, a value of 0 used in a pointer
context is treated as a NULL pointer. So that one line is seen as:

    
    
        if (getuid() != 0 && geteuid == NULL)
    

Now, let's play compiler. geteuid() is a function and it's defined. So the
compiler can generate:

    
    
        if (getuid() != 0 && false)
    

The result of this expression can never be true. But because the call to
getuid() could cause side-effects (the prototype for getuid() does not include
the GCC extension marking it as a "pure" function) the compiler _has_ to emit
a call to the function, but otherwise, it ignores the result since it doesn't
matter what the result is. The resultant code is:

    
    
        int main(void)
        {
          getuid();
          printf("okay\n");
          return 0;
        }
    

And thus, it's possible, even with the commonly used "-Wall -Wextra" options
to GCC. To detect this, you will also need to add "-Wunreachable-code" (why
that isn't included with '-Wall' is a good question).

~~~
CUViper
> "gcc -ansi -Wall -Wextra -pedantic". NOT ONE ERROR OR WARNING!
    
    
      $ rpm -q gcc
      gcc-4.8.2-7.fc20.x86_64
      $ gcc -ansi -Wall -Wextra -pedantic test.c
      test.c: In function ‘main’:
      test.c:7:36: warning: the comparison will always evaluate as ‘false’ for the address of ‘geteuid’ will never be NULL [-Waddress]
             if (getuid() != 0 && geteuid == 0)
                                          ^
    

If fact, just -Wall is enough to enable that warning. Maybe the coverage of
-Waddress has changed since your version?

~~~
spc476
Could very well be. I am using a slightly older version of GCC.

------
claudius
Regarding the Debian OpenSSL disaster, the question _was_ brought up on the
OpenSSL mailing list (and read there); regarding the Tarsnap bug, it is not an
unbraced if but a non-incremented nonce. If two of the five comments in an
article are obviously wrong, how much trust should I possibly put into the
remaining 60%?

~~~
tokenrove
It's sarcasm. You've missed the rhetorical point of the article.

(To be more clear, each comment mimics the comments made about the "goto fail"
bug, and the crucial point is not pure mockery or a defence of Apple, but
rather the final paragraph.)

~~~
thezilch
The point is that because there are always mistakes in everything, that the
NSA is never involved? Strange, because I'm sure they've been implicated more
than zero times.

NOTE: I have no opinion on their involvement in the _goto fail;_

~~~
adamnemecek
> The point is that because there are always mistakes in everything, that the
> NSA is never involved?

I'm pretty sure he's being sarcastic.

------
jodrellblank
Another thing they have in common is trying to reason about security of SSL
certificate chains and other high abstraction concepts in terms of C pointers
and function return values.

------
IvyMike
Maybe that will be my new way to measure the effectiveness of my test suite.

"Imagine the NSA snuck a one line 'fix' into your software overnight. Do your
tests quickly and accurately detect the problem and point to the code that is
broken? If not, your unit tests are broken."

~~~
ghayes
If, per se, the NSA has your codebase, surely they would be able to find
_something_ you haven't tested. Your test cases should be extensive, but it's
impossible to make them exhaustive.

~~~
mpyne
And even if you do, now you have a large and probably-buggy test base.

------
MBlume
Lint tools exist.

My number one take away from this is that _we should all be using them more_.

~~~
azernik
And ignoring compiler warnings less.

~~~
girvo
I only recently learnt C, and -Wall -Wextra -Werror was drummed into me as
being "the right way". It certainly does catch a lot of my idiotic mistakes.

~~~
bhaak
Although be aware that -Wall and -Wextra activate different warnings on
different compilers and even on different versions of the same compiler. E.g.
gcc added lots of warnings to them in the last versions.

So your code might not compile anymore on newer compiler versions. This is not
a problem as such but for example in combination with updates to your
continuous integration environment, this might cause build fails.

~~~
JoeAltmaier
Yeah warnings are out of control. New architectures and platforms bring up new
things to be concerned about, thus new warnings. But old code quits working
solely because of the build issue you mention.

Wonder what the solution would be.

~~~
MBlume
Does C have the ability to locally silence warnings in particular sections of
code?

~~~
kibibu
Yes, but it's compiler dependent.

For GCC, see [http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-
Pragmas.html](http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html)

~~~
JoeAltmaier
Useful, but no solution to the issue of new warnings. You'd have to be able to
say "I just want THESE warning" to guarantee code continues to compile.

------
adamlj
"This is an important lesson to learn: Mistakes can happen any time a piece of
code is modified."

From [http://www.daemonology.net/blog/2011-01-18-tarsnap-
critical-...](http://www.daemonology.net/blog/2011-01-18-tarsnap-critical-
security-bug.html) regarding the Tarsnap bug.

Solution: write more unit tests.

~~~
peterhunt
Or use better abstractions / static analysis.

------
nemothekid
A lot of arguments center around "well, the compiler should have done X!"

I don't write a lot of C code, but when I did, and even more relevant - when I
compile packages, the compiler tends to spit out chapters worth of warnings
that I just ignore. How accepted is compiling C code in practice with Werror?
Could it be the compiler did warn about goto fail, and the OP's error, but it
was 1 warning out of a million? Really interested in an answer from those who
work in C shops.

~~~
Gracana
I write my code with -Wall, -Werror, -pedantic, and a standard specified. If
my code does something questionable, I need to instruct the compiler that I
_really_ mean for it to do that. Why tolerate unsafe and ambiguous behavior?

~~~
bsder
One problem is that -Wall doesn't actually enable all warnings. And that's
nuts.

~~~
Gracana
You can enable -Wextra as well, but there are still more warnings you have to
enable manually. At that point though, you're looking at weird not-
necessarily-bad and language-specific things.

------
Rygu
1\. Start writing tests. Tests will save your ass when you're merging a
feature branch. Cause are you sure everything still behaves correctly after
those ingenious refactorings?

2\. Use an IDE and/or compiler that actually warns you about common human
errors like unused variables, unreachable statements, and dumb mistakes like a
case-block with silent fallthrough to the next case-block.

3\. Acknowledge responsibility when writing security related code. Know that
there's no room for mistakes.

------
matt__rose
with the X bug, building X has always triggered tons of warnings, it's easy to
see why they're overlooked [http://keithp.com/blogs/xserver-
warnings/](http://keithp.com/blogs/xserver-warnings/)

------
julie1
openBSD credo: security bugs are just bugs.

Mostly human, and avoidable but always present because people don't take the
time to do it correctly. Quality, taking the time to make the things right,
slowly, correctly, all that matters more than genius. Quality code at all
level is the key to secure development. Every little things count.

I know a few openBSD developers, and their song about doing the right things,
taking our times to do it correctly - simpler, with quality because it is the
shortest path to correctness thus speed (what is the use of an incorrect
program: none). This song is appealing. The fact they are strangely
psychopathic paranoid nerds is not ;)

I think computer industry would be far better if we would slow down and focus
on doing less software, but better built.

Less functionalities maybe, but the one that maybe more powerful. The stable
foundation for sustainable progress.

And, if you like them -even though they are repulsing antipathic nerds- you
can support them :)

~~~
nayden
careful there. we're not all the same -- some are MUCH more paranoid than
others and I am yet to meet an antipathetic developer. ;)

But thanks to those who support what we do!

------
chetanahuja
To those learning C programming "by reading other's code", following is an
example of what not to do (either the subtracted or the added line).

[Now I wait for a deluge or replies about how it's "idiomatic" C. All I have
to say to that is, s/ma// ]

    
    
      --- libc-a/memset.c
      +++ libc-b/memset.c
      @@ -1,6 +1,6 @@
      void *memset(void *_p, unsigned v, unsigned count)
       { 
           unsigned char *p = _p;
      -    while(count-- > 0) *p++ = 0;
      +    while(count-- > 0) *p++ = v;
           return _p;
       }
    

[Edited later to use the code mode]

~~~
EdwardDiego
What is particularly bad? How would you have done it differently? I've not
done much C, so I can't see the issue, although is it perhaps the types? Would
count be modified by side-effect in the calling context also?

~~~
chetanahuja
_" Would count be modified by side-effect in the calling context also?"_

Questions like this and others (eg., does the pointer deref operation applies
to incremented or non-incremented value of p?) that you have to answer before
you can make sense of the code. The cognitive overload on the reader's brain
is at least part of the reason that a bug like that went past code-review in
memset function on a major platform. MEMSET !! Was the needless cleverness and
terseness of the code worth it?

P.S. The answer to your question is _No_. The variable _count_ as used in the
body of this function is local to this function (it's passed by value... so
it's a copy of what the caller passed).

~~~
EdwardDiego
Cheers mate, ah, so unless the function declares that it takes a pointer then
it's pass-by-val?

~~~
ggreer
Everything is pass by value in C, but pointers are values. Functions can't
modify the value of a pointer, but they can modify whatever's in the memory
pointed to by the pointer. To change the pointer itself, you'd have to pass a
pointer to a pointer. Functions like asprintf do this.

C++ added pass by reference (&), which can be a handy bit of syntactic sugar.

------
chris_wot
Actually, with the Debian SSL "fix" that removed entropy, the dev who made the
change _did_ ask about it on IRC. Though a bad mistake, it's understandable
how it happened.

------
tbmatuka
This one may not be as bad as the ones in the article, but it still gave us a
few weeks of work upgrading kernels on a couple of thousand production
servers:
[http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.g...](http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8176cced706b5e5d15887584150764894e94e02f)

------
ybaumes
In the example of the Apple bug, I've read it took eighteen months (!!) to
discover it after it has been committed in the source code base. As a
consequence, I would be interested in knowing, for each bug, the time it took
to discover them.

Also, I assume that once discovered, the time to fix it is usually negligible.
Usually. And as long as you don't count the time for full deployment in
production...

------
rjzzleep
i'm actually a little surprised about the openssl one. I always assumed it
would be good practice to number error codes negatively. OTOH that's also
kinda expected behavior for reverse engineers. if the return value is negative
you can be almost certain it's an error code

i guess whoever wrote that line thought the same.

------
jonalmeida
I've come across a bug that was probably caused by a developer that held the
Ctrl key while pressing the up arrow which caused the selected line to move
above another.

It luckily only took me a full day to find within the codebase..

------
dhugiaskmak
"Never write your own security code, because you'll get it wrong. Leave it to
the smart people."

