Even DJB's code is buggy. (And some others too.) | Hacker News Reader

Even DJB's code is buggy. (And some others too.)

google.com

18 pointsdgn19 comments

DJB only sets errno_intr here in error.c

  int error_intr =
  #ifdef EINTR
  EINTR;
  #else
  -1;
  #endif

The (errno = error_intr) happens in this block:

   default:
      for (;;) {
	r = read(fdfile,inbuf,sizeof inbuf);
	if (r == -1) {
	  if (errno = error_intr) continue;
	  _exit(23);
	}
	if (r == 0)
	  break;
	if (!fetch_ascii)
	  substdio_put(&ss,inbuf,r);
	else
	  for (i = 0;i < r;++i) {
	    if (inbuf[i] == '\n')
	      substdio_put(&ss,"\r",1);
	    substdio_put(&ss,inbuf + i,1);
	  }
      }
      break;

Like a lot of DJB code, it's hard to tell exactly what he's trying to do. He could be trying to deal with some weird platform. Where EINTR is defined to something other than 0 that code will cause the read() to be repeated whenever it returns -1 and when the code eventually gets back to the caller errno will be left set to EINTR. But on platforms where EINTR is 0, a read error will exit.

You're right that this could theoretically be the intent -- but it's very unlikely that it's necessary: "The value of errno is zero at program startup" - ISO/IEC 9899:TC2. If EINTR was 0, there'd be no way of detecting it.

Yet another example of why you should always compile with warnings as errors and warnings set as high as you can tolerate. Oh, and if you have a static type system, you might as well use it by preventing implicit casts to bool.

I'm assuming this bug is an instance of a more general one, where people write if(... = ...) when they meant to use == instead?

Hence, here, error-handling code will be improperly invoked irrespective of whether they get a given error code or not.

> I'm assuming this bug is an instance of a more general one

Anyone want to work on a regexp for the more general condition?

Most of these don't look like bugs, but here's a search to match "if (... = ":

http://www.google.com/codesearch?hl=en&lr=&q=if\+\(\w%2B\+%3D\+&sbtn=Search

Can anyone think of a better way to match the subset that are more likely to be bugs?

http://www.google.com/codesearch?hl=en&lr=&q=if\+\(\w%2B\+%3D\+[^(\.]%2B\+%3F\)&sbtn=Search

That takes out parentheses (for function calls) and structure member assignment because it might be two statements in one line (ugly and error prone as output will be exactly the same for a = b.c; if(a) {}; vs. if (a=b.c)

> whether they get a given error code or not.

Helpfully, the Emacs code is commented, which helps explain the pattern:

   if (errno = ack.hf_retcode)   /* set errno based on returned */
     return (-1);                /* code, if 0, then no error   */

Presumably errno is a global, and when it is set to a value other than zero, the function returns. So, it isn't an example of = being used in place of ==, at least this snippet appears to be correct.

I have for years made it a habit to write "if (CONSTANT == variable)" rather than "if (variable == CONSTANT)" so that if (oh, let's be honest -- when) I accidentally use "=" instead of "==" I'll get a big nasty compiler error.

Some people have commented that it "looks kinda backwards," and I'm not sure what to make of that. My "backwards" way is perfectly correct, C-wise and math-wise, it doesn't make the code hard to understand, and it solves a real problem.

But I appreciate that it's important not to upset the other people on your team willy-nilly. So it's a bit of a quandry.

Other things to prevent this error: 1) Don't ever use assignment in a conditional expression, even on purpose. 2) Aggressively declare everything as const, even scalar input parameters to your function. 3) Use code analysis tools that treat this as a warning, which you should be treating as an error.

I'm sorry, who (or what) is DJB and what codebase are you referring to exactly? Also, what is the bug there? Can anyone educate me?

Daniel Bernstein (qmail, djbdns, etc): http://cr.yp.to

The bug searched for is using the assignment operator = instead of the equality operator ==.

Ha, thanks! I didn't notice '=' was wrong there, part of me thinks that can actually be common in C (assigning the result of a condition to be further used within the block).

It can be intentional if the right side is a function call.

But something like:

if (errno = 0) { blah(); }

is almost certainly a bug.

Despite ugly style, this is not a bug actually, assuming that error_intr is meaningful, and continue should be called only if error_intr is false:

if (errno = error_intr) continue;

error_intr is meaningful, but it's defined as the value of EINTR, which can never be zero -- so this is a bug.

This was caught here:

http://www.nabble.com/Bug-in-fetch.c:doit--td1143190.html

Some of those look like instances of a local variable called errno being used as a return code.

That's OK, it'll just shadow the global errno in that func, no?

I don't remember hearing DJB claim that his code was free of bugs, only that it was free of security holes.

Reply on news.ycombinator.com