GCC 6: -Wmisleading-indentation vs “goto fail;”

I work at Red Hat on GCC, the GNU Compiler Collection. The next major release of GCC, GCC 6, is just around the corner, so I thought I’d post about a new compiler warning I’ve contributed to it:
-Wmisleading-indentation.

GNU logo

One of the more common “gotchas” in C and C++ programming relates to missing braces. For example, in the following:

  if (some_condition ())
    do_foo ();
    do_bar ();

the “do_bar ();” looks like it’s guarded by the conditional, but it’s actually outside of it.

Similarly, in this code fragment:

  for (i = 0; i < n; i++)
    sum[i] = a[i] + b[i];
    prod[i] = a[i] * b[i];

the computation of prod[i] is actually outside of the loop, despite what the indentation might suggest.

Perhaps the most famous example of this is the so-called “goto fail” vulnerability that affected OS X’s SSL implementation, aka CVE-2014-1266:

    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail;
    /* more checking here.  */
  fail:
    /* cleanups */
    return err;

where the 2nd goto fail; was always executed, leading to signature-checking code being skipped, with “err” set to 0, effectively leading to invalid certificates being accepted as valid.

This felt to me like something the compiler ought to warn about, so for gcc 6 I’ve written a new warning: -Wmisleading-indentation.

The new warning is issued when the indentation of the code might mislead a human reader about the code’s true block structure.

For example, given CVE-2014-1266 above, gcc 6 will issue this:

sslKeyExchange.c: In function 'SSLVerifySignedServerKeyExchange':
sslKeyExchange.c:631:8: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
        goto fail;
        ^~~~
sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is not
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    ^~

Similarly, for the second example above:

test.c: In function ‘example_2’:
test.c:57:5: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
     prod[i] = a[i] * b[i];
     ^~~~
test.c:55:3: note: ...this ‘for’ clause, but it is not
   for (i = 0; i < n; i++)
   ^~~

At a high level, the underlying implementation looks at control statements (if/else, while, for), and if it sees them guard a single statement without braces, it looks at the followup statement. It complains if both have the same indentation.

That’s a simplified description – we spent a fair amount of time working on heuristics in the warning, to try to ensure that it warns for all cases that are reasonable to warn for, whilst not complaining unduly for indentation that’s merely bad (rather than being actively misleading). We’ve also tested it with a variety of coding styles: GNU, K&R, Linux kernel, etc.

For example, it will warn about poorly-written one-liners:

  if (flag)
    x++; y++;

like this:

  test.c: In function ‘example_3’:
  test.c:25:10: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
     x++; y++;
          ^
  test.c:24:3: note: ...this ‘if’ clause, but it is not
   if (flag)
   ^~

but it doesn’t warn for things like:

  void all_on_one_line (void) { foo (0); if (flagA) foo (1); foo (2); foo (3); }

I added the new -Wmisleading-indentation warning to “-Wall” in gcc 6’s development tree in December, and it’s been finding real world bugs, for example one in elfutils: https://gnu.wildebeest.org/blog/mjw/2016/01/09/looking-forward-to-gcc6-nice-new-warnings/
and a couple in the Linux kernel’s “perf” tool:
* https://lkml.org/lkml/2015/12/14/460
* https://lkml.org/lkml/2015/12/14/461

GCC 6 will be available in Fedora 24:
https://fedoraproject.org/wiki/Changes/GCC6
and in other fast-moving distributions.

Alternatively, if you’re feeling adventurous, you can download a development snapshot of gcc 6 from https://gcc.gnu.org/snapshots.html

Enjoy – I hope you don’t find any such warnings in your code!


Join Red Hat Developers, a developer program for you to learn, share, and code faster – and get access to Red Hat software for your development.  The developer program and software are both free!

  1. Does it warn on
    if (some condition);
    foo();

    ?
    The indentation suggests that foo() is only executed if some_condition is true, but the semicolon already terminates the if().
    I have seen that a few times.

    Like

  2. > but it doesn’t warn for things like:
    >
    > void all_on_one_line (void) { foo (0); if (flagA) foo (1); foo (2); foo (3); }

    Ah, indeed. Obviously a later refinement, after I had already rewritten such across the LibreOffice code base for an earlier version of GCC 6. (Rewritten from “merely bad” to “arguably more readable,” though, so who’s to complain?)

    Like

  3. Thanks for the nice feature! Does it also catch cases like:

    if (A)
    if (B) stuff();
    else other_stuff();

    vs

    if (A)
    if (B) stuff();
    else other_stuff();

    ? That’s one that’s bitten us before.

    Like

  4. Nice feature but, please, take it out of -Wall. It produces way too many false positives, and is way too complicated, for what is many times a matter of style, not correctness.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s