vade retro Pascal - redundant parentheses revisited

Werner Almesberger werner at
Tue Oct 22 12:01:10 EDT 2013

[ Came across a security-related article that made me think of an
  old and constant gripe of mine. Here's my rant, somewhat off-topic. ]

I've been on a crusade against redundant parentheses for a long time.
They often make it hard to read other people's work and obscure the
programmer's intentions.

A very common use of redundant parentheses in C occurs in Boolean
expressions, e.g.,

	if ((a > 10) || (a == 0))

The origin of such constructs my be Pascal. In Pascal, the Boolean
operators "and" and "or" have higher precedence than relational
operators. Therefore, in Pascal, all the parentheses in

	if ((a > 10) or (a = 0))

are required. Note that Pascal uses "=", not "==", to test for

C gives lower precedence to && and || than to relational operators
and we therefore don't need parentheses in this case:

	if (a > 10 || a == 0)

Clean and simple.

Programmers coming from a Pascal background may have brought these
parentheses to C, corrupted those improperly schooled in C operator
precedence, and thus caused this now widespread disease. If you
think of it, it's a bit like how the Black Death came to Europe.

So far, these things are widely known. Now, there's a twist:

Once upon a time, a malicious "bug" was introduced in the Linux
kernel that made use of this sloppy practice to hide its nefarious
purpose. While I didn't make the connection to redundant parentheses
back then, I recently read an article [2] that rehashed the topic,
and spotted the link this time. The code in question looked like

	if ((options == (__WCLONE|__WALL)) && (current->uid = 0))

This is structurally equivalent to

	if ((a > 10) || (a = 0))

Note that the second expression is an assignment, not a comparison.
In the example above, the intent is to give a process root privileges
by setting current->uid to zero if there is a certain "magic" pattern
in the "options" argument.

"==" vs. "=" is a common typo in C and can produce nasty and hard to
find bugs. It is so common that modern C compilers warn when seeing
things like

	if (a = 0)

gcc -Wall:

  warning: suggest parentheses around assignment used as truth value

LLVM (clang) gives the following lecture:

  warning: using the result of an assignment as a condition without parenthese
  note: place parentheses around the assignment to silence this warning
  note: use '==' to turn this assignment into an equality comparison

As LLVM explains, the warning can be silenced by adding parentheses.
This works with both compilers:

	if ((a = 0))

Looks ugly and should serve as a hint that it's better to avoid this
sort of assignments entirely. E.g., instead of

	if ((a = EXPRESSION))

to write

	if (a)

Now, the double parentheses are easy to spot. But, given the
widespread bad practice of using redundant parentheses, it's much
harder to catch the assignment in the malicious case:

	if ((a > 10) || (a == 0))	/* ugly but common */


	if ((a > 10) || (a = 0))	/* evil */

This could of course also happen as the result of a normal typo. So
by using redundant parentheses, developers not only make their code
harder to read, they also defeat a mechanism that is there to protect
them from likely mistakes.

Let's see what would happen if using only the parentheses C requires:

	if (a > 10 || a = 0)
  error: lvalue required as left operand of assignment
  error: expression is not assignable

Both are right - the expression now becomes the equivalent of

	if ((a > 10 || a) = 0)

and the left-hand side of the assignment makes no sense. That's also
why the malicious code really needed the parentheses: not only to
avoid a warning that may draw undesired attention, but to get the
code to compile at all.

But if the expressions were swapped, the code would be syntactically

	if (a = 10 || a > 10)

And now, the compiler's warning is the last line of defense.

- Werner


More information about the discussion mailing list