Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 4:48 UTC (Mon) by kalvdans (subscriber, #82065)
Parent article: Kernel prepatch 3.11-rc4

A mysterious security fix indeed:

+#define is_gate_vma(vma) ((vma) = &gate_vma)

This can never be correct.

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 10:29 UTC (Mon) by NAR (subscriber, #1313) [Link] (29 responses)

When I started learning C, one of the useful tips I got was that when I check equality between an rvalue and and lvalue, always put the rvalue on the left, so if I miss the second =, the compiler will catch it. It might seem ugly, but damn useful.

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 11:01 UTC (Mon) by lkundrak (subscriber, #43452) [Link] (8 responses)

Yoda condition it is.

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 11:57 UTC (Mon) by hummassa (guest, #307) [Link] (7 responses)

People usually dislike Yoda conditions, and argue that compiler warnings are effective in avoiding this kind of mistake, but I think it is elegant.

I am obssessed with the whole "repeat until silently compile with -Wall" thing, and yet I think it is a good sign of humility ("I have got this wrong once or twice or a hundred time, I'll be on the watch from now on").

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 12:02 UTC (Mon) by mpr22 (subscriber, #60784) [Link]

I'd say that it's structurally elegant, but not visually elegant (and that the solution-by-good-design would have been to have := as assignment, == as comparison, and = as a syntax error).

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 13:14 UTC (Mon) by error27 (subscriber, #8346) [Link] (3 responses)

Messy code is like a dripping faucet or a toothache. It shouldn't make you irritable but it does. People don't go around talking like Yoda, it's annoying.

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 14:22 UTC (Mon) by hummassa (guest, #307) [Link] (2 responses)

My native language (Brazilian Portuguese, the "last flower of the Latium, uncultured and beautiful") has an olympic-gymnast flexibility towards the positioning of the elements in a phrase, and that flexibility has many poetic uses. So, to many poetry-inclined Brazucas like me, it's probably not as annoying.

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 15:51 UTC (Mon) by Cyberax (✭ supporter ✭, #52523) [Link] (1 responses)

Lots of synthetic languages (Slavic ones, most notably) have completely flexible word order. Yoda-speak sounds contrived in Russian, but not that much unusual - similar sentence constructions are used in official speech and in poetry.

Still, Yoda-assignment looks somehow unappealing.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:16 UTC (Tue) by dgm (subscriber, #49227) [Link]

Pretty much, a yoda-assignement would not work at all :-P

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:14 UTC (Tue) by dgm (subscriber, #49227) [Link] (1 responses)

Another good sign of humility is using different compilers, as each usually has different sets of warnings. You actually learn a lot about the real C language (as opposed to the naive simplification one has in his mind when learning) by fixing pedantic compiler warnings.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 11:54 UTC (Tue) by hummassa (guest, #307) [Link]

Oh, I am quite humbled by the sheer number of moving parts that autotools introduce in any simple program... :D

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 12:11 UTC (Mon) by error27 (subscriber, #8346) [Link] (19 responses)

Don't do that. Or if you prefer: Don't that, do.

These bugs are extremely rare since the compiler normally warns about them if you don't use the double parenthesis. Probably we get one or two of these per year in the kernel. Meanwhile Yoda code makes the code ugly and hides more bugs than it fixes. We don't write Yoda code in the kernel.

Along side the GCC warning I have a static checker that looks for bugs like:

if ((x = 10)) {

I will amend it to look for:

if ((x = &foo)) {

since that is obviously never false. Also this should trigger an unreachable code warning... Hm...

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 12:20 UTC (Mon) by error27 (subscriber, #8346) [Link] (8 responses)

Also in the kernel checkpatch.pl doesn't let people do:

if ((rc = lbmRead(log, log->page, &bp)))

Banning these is better than writing Yoda code because it makes the code easier to read and less error prone at the same time.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:23 UTC (Tue) by dgm (subscriber, #49227) [Link] (7 responses)

How do you catch this, then:

  some_function(x = what_ever, more_args);

specially when "x = ..." comes from macro expansion?

Yoda conditions are VERY useful. You don't even need an static checker. In forcing yourself to slowdown to write the condition reversed, _you_ will catch the mistake before it even reaches the compiler. Worse is better.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:13 UTC (Tue) by mpr22 (subscriber, #60784) [Link] (5 responses)

rvalue-first comparison is a clever and useful workaround, and in the guts of a macro, it's clearly the right thing. It's also deeply jarring when I read it, so I'm not sure it's the right thing in the main body of the code.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:59 UTC (Tue) by khim (subscriber, #9252) [Link] (4 responses)

It's obviously wrong thing to do because code readability is more important then code writability: it's written once by one guy but then read by thousands and sometimes refactored (to refactor something you need to first read old code and then write new code thus balance is unchanged).

Tradeoffs may be different in code which you are writting for yourself to only run it once... but you would not believe how often such code survives for years, so I'm not sure there are places where I would like to see Yoda conditions in practice.

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:21 UTC (Wed) by dgm (subscriber, #49227) [Link] (3 responses)

What's the value of code that's easy to read, but incorrect?

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:32 UTC (Wed) by mpr22 (subscriber, #60784) [Link] (2 responses)

Low, possibly negative. But once you fix it, it is code that is both correct and easy to read, making it superior to the code that is correct and hard to read.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 13:22 UTC (Wed) by dgm (subscriber, #49227) [Link] (1 responses)

For this you first have to spot the error. Using one style you get help from the compiler, while with the other you need angry users and some debugger voodoo.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 15:25 UTC (Wed) by mpr22 (subscriber, #60784) [Link]

The compiler can be (fairly easily) modified to alert the user to the errors that the order of comparison operands people like me find natural makes possible. People-like-me's brains cannot be easily modified to find the order of comparison operands people-like-you find sufficiently cautious natural.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 10:58 UTC (Tue) by error27 (subscriber, #8346) [Link]

We don't often use a compare as a function parameter in the kernel so it's not a problem in practice. We don't like it when people use assignments as a parameter. If I spot an assignment like that in a patch, I will tell the submitter to fix it in a follow on patch. But now I will write a Smatch check to spot it automatically so I can complain every time.

I have reviewed the kernel patches so far this year. Out of 39,745 total patches, there are 21 patches where only a '=' character changed. There is only one '=' vs '==' typo and this ARM bug will be the second bug like that this year. The remaining 20 bugs from the list are almost all off-by-one bugs.

The '=' vs '==' patch was: 9e48854c58 "drm/tilcdc: Fix an incorrect condition". It was found with by checkpatch.pl. Checkpatch is a mandatory step for patches and it reflects a problem in the process that it was skipped for this patch.

It should also have been caught by GCC except the authors of that driver put extra parenthesis around everything as if they are programming in LISP. In the kernel, LISP-isms are considered bad style. We don't have an automated process to complain about this, but I do try to educate submitters to drivers/staging/ about this.

The tilcdc bug would have been caught by Smatch as well but it is an ARM thing. The new CONFIG_COMPILE_TEST might help with this in the future.

Apparently the driver was tested, but I have to think that it wasn't tested very well. The maintainer, Rob Clark, did not have a board to test it on at the time.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:32 UTC (Tue) by kugel (subscriber, #70540) [Link] (5 responses)

How can a yoda condition possible "hide more bugs than it fixes"?

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 10:01 UTC (Tue) by khim (subscriber, #9252) [Link] (4 responses)

Easy: when you see on Yoda condition it's harder for you to notice that code does something syntactically correct yet semantically wrong. Uses address of wrong variable (which is pushed to the end of expression with Yoda condition), for example.

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:25 UTC (Wed) by dgm (subscriber, #49227) [Link] (3 responses)

How can you tell if an expression is correct if you don't read them in full?

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:32 UTC (Wed) by mpr22 (subscriber, #60784) [Link] (2 responses)

You can't. Which is why making expressions hard to read is a bad idea.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 13:24 UTC (Wed) by dgm (subscriber, #49227) [Link] (1 responses)

Which of those expressions is harder to read:

A == B

B == A

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 15:18 UTC (Wed) by mathstuf (subscriber, #69389) [Link]

Let's use some actual examples:

> i == container.end()
> container.end() == i

> result == (2 * idx * idx)
> (2 * idx * idx) == result

> pivot < container.size() / 2
> container.size() / 2 > pivot

> speed > threshold
> threshold < speed

In all of these cases except the last, I would naturally write the first line[1]. I think they read more naturally. To reduce errors, I have const on every variable possible (containers I have to build are the exception (I can't wait for initializer lists)), so my code tends to follow SSA a lot[2].

[1]I try and order all orderings to use '<' since I think "(a > min) && (a < max)" is harder to parse and using one comparison everywhere helps keep the mental overhead low.
[2]It helps in debugging since all intermediates are named and you don't get things like i->first.second.second in the middle of lines and have to backtrack what the iterator's pair nesting is all the time.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:55 UTC (Tue) by khim (subscriber, #9252) [Link] (3 responses)

Also this should trigger an unreachable code warning...

You would not believe how often similar code is used in metaprogramming to strip away code which is not needed in some cases because we are using regular variable and not something more complex.

Sure, new warnings are added to compiler but that's not an easy process: even simple things like open(2) do something like this in modern system (try to look on /usr/include/bits/fcntl2.h in your system sometime, you'll be surprised to see how open(2) is implemented in modern GlibC).

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 11:05 UTC (Tue) by error27 (subscriber, #8346) [Link]

In Smatch, I print a list of unreachable code and then run a script over it to remove the kernel specific false positives.

Kernel prepatch 3.11-rc4

Posted Aug 10, 2013 11:15 UTC (Sat) by kleptog (subscriber, #1183) [Link] (1 responses)

I had a look and indeed, open(2) is implemented quite differently from what I expected. It implements error checking on the arguments and there's even a (not very well documented) __errordecl() function which allows you to add new error messages to gcc. I can probably find a use for that.

Thanks for the tip.

Kernel prepatch 3.11-rc4

Posted Aug 10, 2013 11:38 UTC (Sat) by khim (subscriber, #9252) [Link]

Small correction: __errordecl is not function, it's a macro defined in /usr/include/sys/cdefs.h and there are also similar __warndecl ...

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 23:24 UTC (Mon) by xtifr (guest, #143) [Link] (2 responses)

Wow, do the kernel standards not support/require inlines for cases like this? Inline has been available in gcc for god-knows-how-long, and has been part of the C standard since C99.

inline int is_gate_vma(whatever* vma) { return vma = &gate_vma; }

This should get a compile-time error, since it's not returning an int/bool, or at the very least, a stern warning, and possibly another warning, since it only modifies a local variable. (To actually do an assignment, you'd have to pass a pointer to the pointer.)

The macro version, by contrast, will suppress any warning the compiler might issue, because it includes the extra parens that tell gcc this sort of thing is ok--and has to include those extra parens. Macros may be *less* evil in C than in C++, but they're still something I prefer to avoid when I can.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:30 UTC (Tue) by kugel (subscriber, #70540) [Link]

Good point. It should have been an inline in the first place.

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 11:18 UTC (Wed) by cesarb (subscriber, #6266) [Link]

Why do we have to keep using int as if it were bool? Why not "inline bool is_gate_vma(...)" instead of "inline int is_gate_vma(...)"?