Fixing programmers

Posted Mar 15, 2019 21:36 UTC (Fri) by madscientist (subscriber, #16861)
In reply to: Fixing programmers by myxie
Parent article: Cook: security things in Linux v5.0

Continue and return work the same inside a switch statement as outside a switch statement. They are not special.

The main problem is K&R chose the wrong "default" behavior. Usually you DON'T want to continue, so that should have been the default (that is, the current case is automatically terminated by the next case or the end of the statement), and there should be a special keyword used only when you DO want to fall through.

The one exception is empty cases ("case a: case b: case c: foo(); break;") A special dispensation could have been made that the keyword is not needed to fall through unless there are other statements in the case: empty cases don't need the keyword.

Of course, you need a different keyword. They could have used "continue", but both "break" and "continue" are already useful from within switch statements (to break out of or continue an enclosing loop) so using either one of them as meaningful inside a switch was (IMO) not a good decision. I've definitely run into situations where I wanted to break out of a loop from within a switch statement and having "break" overloaded is annoying.

A "fallthrough" or "nobreak" keyword would have been fine. Or if you REALLY didn't want to add a keyword you could have played tricks with "goto", maybe, allowing "goto case X;" where "case X:" existed somewhere in the current switch statement, plus "goto default;"

But that's not the language we have, so FALLTHROUGH-type comments that are recognized by the compiler are great things and everyone should use them. Even if they're weren't recognized by the compiler, they're recognized by the next programmer to come along and read the code, and make clear that you didn't forget something, so they should still always be used.

Fixing programmers

Posted Mar 15, 2019 21:44 UTC (Fri) by rweikusat2 (subscriber, #117920) [Link] (22 responses)

> But that's not the language we have, so FALLTHROUGH-type comments that are recognized by the compiler are great things and
> everyone should use them. Even if they're weren't recognized by the compiler, they're recognized by the next programmer to come > along and read the code, and make clear that you didn't forget something, so they should still always be used.

They make clear that someone was using lint (or similar). Not particularly interesting and technically useless as the default case is - well - the default case. Whether or not this is correct is a different conversation, but that's something which can only be determined by looking at the code. There's no more reason to assume that two cases with fallthrough are correct or incorrect than two cases seprated by a break;. I've actually written code where I forgot to omit the break in the past :-).

Fixing programmers

Posted Mar 15, 2019 22:42 UTC (Fri) by madscientist (subscriber, #16861) [Link] (20 responses)

> They make clear that someone was using lint (or similar). Not particularly interesting and
> technically useless as the default case is - well - the default case. Whether or not this is
> correct is a different conversation, but that's something which can only be determined by
> looking at the code. There's no more reason to assume that two cases with fallthrough
> are correct or incorrect than two cases seprated by a break;. I've actually written code
> where I forgot to omit the break in the past :-).

I'm not going to get drawn too far into this conversation because it's ridiculous, but I'll say that the above is at best missing the point.

A fallthrough comment is not trying to tell you whether or not the code will fall through. Obviously it will, there's no break! The comment is telling you that someone thought about it and INTENDED the fallthrough to happen.

If lint or the compiler warns someone about a missing break and they want a break, they'll obviously just add a break. If they get a warning and they intended to fallthrough, they will add a fallthrough comment. If they didn't add either one then you don't know whether they thought about it and intended the fallthrough, or they just forgot.

Of course, even with the comment the fallthrough behavior could be a bug. Code has bugs. But the comment is not telling you the code is bug-free, it's telling you that it's intentional, rather than an omission. That's still a big benefit.

Fixing programmers

Posted Mar 17, 2019 20:29 UTC (Sun) by rweikusat2 (subscriber, #117920) [Link] (19 responses)

> A fallthrough comment is not trying to tell you whether or not the code will fall through. Obviously it will, there's no break! The
> comment is telling you that someone thought about it and INTENDED the fallthrough to happen.

It's usually safe to assume that code came to be in a certain form because someone was convinced it would be the right way to solve a particular problem. But that's not particularly interesting. Interestsing is "is it the right way".

Fixing programmers

Posted Mar 18, 2019 13:49 UTC (Mon) by mathstuf (subscriber, #69389) [Link] (18 responses)

Depends. Sometimes "uninteresting" parts of a patch are done at a late (or early) hour and is just more prone to typos and errors. Reviews are invaluable to every coder, from those learning anew all the way up to Linus. One of the first things I do after opening a merge request is do another look over the code. I've lost count of how many 5-minute-later pushes I've made because of that. Or if I find a bigger issue, I at least leave a comment that something looks weird and might need more thought.

Fixing programmers

Posted Mar 18, 2019 16:14 UTC (Mon) by jezuch (subscriber, #52988) [Link]

I do that too, *and* I do the initial commit using git commit -p as a kind of self-review. Catches plenty of stuff I wanted to do or did temporarily but forgot etc.

Fixing programmers

Posted Mar 18, 2019 19:40 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link] (16 responses)

I don't understand what that's supposed to mean in the given context.

I was trying to express two things:

1) Noting that a break is absent isn't sufficient grounds to assume that this must have been an oversight unless there's an a priori conviction that this is a very likely cause for an absent break. Considering the "99% false positives", such a conviction doesn't seem sensible to me.

2) Computers execute code as it was written, not as it was intended to be written. An absent break which was an oversight isn't necessarily an error. And neither is a conscious omission or a present break necessarily correct.

Fixing programmers

Posted Mar 18, 2019 19:53 UTC (Mon) by mathstuf (subscriber, #69389) [Link] (15 responses)

As a reviewer, I would want a positive indication of intent for either behavior. A lack of either break or a fallthrough comment/attribute is an automatic request for "please clarify". The goal isn't to make code writer's jobs easier, but those who have yet to come and read the code.

Fixing programmers

Posted Mar 18, 2019 21:46 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link] (14 responses)

As I've already explained: Intent doesn't matter. Correctness does. YMMV.

Fixing programmers

Posted Mar 18, 2019 22:11 UTC (Mon) by Cyberax (✭ supporter ✭, #52523) [Link] (9 responses)

Actually you got it wrong. Correctness doesn’t matter. The intent does.

Fixing programmers

Posted Mar 18, 2019 22:29 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link] (8 responses)

If you think so, that's your opinion and not mine.

As person who spends a seriously lot of time working with other people's code (and has done so for about 15 years), I can assure you that I don't give $random_small_quantity_of_money for documentation of "programmer intent", especially not in form of otherwise uninformative comments. I need to know what the code does, not what someone believed it should be doing.

Fixing programmers

Posted Mar 18, 2019 22:33 UTC (Mon) by Cyberax (✭ supporter ✭, #52523) [Link]

> I need to know what the code does, not what someone believed it should be doing.
So you need to know the intent. Duh. You're just deluding yourself at this point.

Correctness checking is TRIVIAL, it's not even worthy of mentioning. Checking of intent is anything but. And that's exactly why modern computer languages try to make it easier for developers to express their intent through code.

Fixing programmers

Posted Mar 18, 2019 22:39 UTC (Mon) by sfeam (subscriber, #2841) [Link]

I believe you have this backwards. What the code does can be determined in the absence of comments. The intent, not so much. That is why comments are valuable for bug-finding. Any place where the documented intent does not match the observed actual behavior is candidate for causing problems.

Fixing programmers

Posted Mar 19, 2019 12:24 UTC (Tue) by anselm (subscriber, #2796) [Link] (5 responses)

The goal of computer programming is to write code that does what it is supposed to be doing. You can use the code itself to figure out what it does, but you can't use the code itself to figure out whether it does what it is supposed to be doing.

It's very easy to write code that does something. Writing code that does what it it supposed to do is a lot harder, and requires outside context so you can determine when you're done. This is why in Real Life™ we have comments, specifications, unit tests, and so on – all to be able to figure out whether code does what it is supposed to do.

Fixing programmers

Posted Mar 19, 2019 14:14 UTC (Tue) by mathstuf (subscriber, #69389) [Link] (4 responses)

There are a few fields where I can see the actual behavior of the code is of utmost importance, damn the intent. Static analysis and zero day hunting to name a few. In these cases, what the intent was actually doesn't matter and correctness is all that counts. However, these fields usually result in changes to the analyzed code when something "interesting" is found, so intent can matter again at that point.

Fixing programmers

Posted Mar 19, 2019 14:31 UTC (Tue) by NAR (subscriber, #1313) [Link] (3 responses)

what the intent was actually doesn't matter and correctness is all that counts.

How can you check correctness when you don't know what that code is supposed to do?

Fixing programmers

Posted Mar 19, 2019 14:43 UTC (Tue) by mathstuf (subscriber, #69389) [Link] (2 responses)

For zero-day hacking, the intent doesn't matter. All that matters is that you can thread your special arguments through to some internal state that allows you access to something else that you normally wouldn't be able to access. In *fixing* the issue, intent again enters the picture since just closing the hole naïvely can block some use case that is supposed to be intended (or break some subtle backwards compatibility). Static analysis is similar. It's goal is to detect what the actual code does, match it against code smells and flag such code. Intent then comes in when you either suppress the notice or change the code to make the code not wrong.

Fixing programmers

Posted Mar 19, 2019 14:55 UTC (Tue) by NAR (subscriber, #1313) [Link] (1 responses)

that allows you access to something else that you normally wouldn't be able to access.

You're making the assumption that this particular access was not intended. So intent matters.

Fixing programmers

Posted Mar 19, 2019 14:59 UTC (Tue) by mathstuf (subscriber, #69389) [Link]

Granted, but I feel that the intent that matters is a much higher-level construct in black hat hacking than "should this case statement have a break statement?".

Fixing programmers

Posted Mar 19, 2019 10:02 UTC (Tue) by NAR (subscriber, #1313) [Link] (3 responses)

Intent doesn't matter. Correctness does.

Let's say I have this code snippet: a = b + c; Is it correct?

Fixing programmers

Posted Mar 19, 2019 21:44 UTC (Tue) by neilbrown (subscriber, #359) [Link] (2 responses)

> Let's say I have this code snippet: a = b + c; Is it correct?

Yes it is.
Your statement "I have this code snippet: a = b + c" is self-evidently correct.

Fixing programmers

Posted Mar 19, 2019 22:03 UTC (Tue) by pizza (subscriber, #46) [Link] (1 responses)

By that definition, the Therac-25 "correctly" killed three people.

Fixing programmers

Posted Mar 19, 2019 23:24 UTC (Tue) by neilbrown (subscriber, #359) [Link]

> By that definition, the Therac-25 "correctly" killed three people.

Sorry to correct you, but I think you mean "It is correct that 'The Therac-25 killed three people' ".

And this is the point - when people start using broad terms like "intent" and "correct" without ensuring that all corespondents are using them in the same sense, you can hardly expect a useful conversation to result.

(wouldn't it be great if people would think about what they write, instead of just writing about what they think).

Fixing programmers

Posted Mar 19, 2019 10:37 UTC (Tue) by HelloWorld (guest, #56129) [Link]

> There's no more reason to assume that two cases with fallthrough are correct or incorrect than two cases seprated by a break;. I've actually written code where I forgot to omit the break in the past :-).
There are actually two reasons:
– the case where you want to use break is vastly more common than the case where you want to fall through
– it's easier to forget adding the break keyword than it is to accidentally add the fall-through attribute

> I've actually written code where I forgot to omit the break in the past :-).
The article mentions 20 cases of missing break statements that were uncovered by this work, while it doesn't mention any cases like the one you describe. It's safe to say that missing break statements are a much more frequent occurrence.

Fixing programmers

Posted Mar 15, 2019 23:15 UTC (Fri) by mjg59 (subscriber, #23239) [Link]

C# blocks implicit fallthrough and requires an explicit goto statement referencing another switch label. goto might be considered harmful in general, but its use there seems consistent with the way it's generally used in the kernel.

Fixing programmers

Posted Mar 16, 2019 23:53 UTC (Sat) by Wol (subscriber, #4433) [Link] (1 responses)

Maybe we could add an optional keyword?

switch nofallthrough {
case a:
case b:
}

As for break being overloaded, can't remember the language but it was something like

labela: for i = 1 to 10
labelb: for j = 10 to 1 step -1
do something
if x continue labela
next
next

so break or continue could take labels to indicate which construct they were meant to break out of.

Cheers,
Wol

Fixing programmers

Posted Mar 17, 2019 0:28 UTC (Sun) by ABCD (subscriber, #53650) [Link]

Among possibly other languages, Java allows this:

labela:
for (int i = 1; i <= 10; i++) {
    labelb:
    for (int j = 10; j >= 1; j--) {
        do_something(i, j);
        if (x) continue labela;
    }
}