Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Google broke a conditional statement that verifies passwords on Chrome OS

arstechnica.com

Post reply on HN
Print view View original

131–140 of 276 posts

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#131

Post by Veserv »

anyfoo wrote: post #79
sneak wrote: post #41

Earlier quoted context omitted.

I would say this is negligence, but perhaps not extremely so.

You are all very quick to judge. I don't work for Google so I don't know what happened, but consider the possibility that this may have been a freak accident. That piece of code for example might only be on a path that is not activated when testing, or it may be executed but not have the same fatal effect. And before anyone comes along and says "well, every possible path should be tested then": That would be absolute…

There are freak accidents and then there are complete lapses is basic critical functionality that demonstrate a total lack of competence.

The article indicates that every person who updated to the latest version could not login. This is a bug that clearly presents itself, every time, in essentially every use case, and is perfectly reproducible. It affects critical functionality that any layperson can recognize is one of the most critical components of a general purpose computing system. The greatest degree of scrutiny should be applied to this system, yet what we see is the functional equivalent of a car factory forgetting to put a wheel on every car it produces.

Sure, there could be a freak accident that makes the 4th wheel robot malfunction, but other stages of the process should catch such an egregious visible failure. Any process that would allow such basic errors to occur without checking or fixing them elsewhere is terrible on its face.

If that is too abstract, imagine you hired a construction company to build a house and they forgot to put in an entire wall. Maybe you just got unlucky and you were the 1 in 1 billion person who gets such a stupid error to happen to them, but the much more likely theory is that they are grossly incompetent, or at the very least far less reliable than you were led to believe.

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#132

Post by kbelder »

JonathonW wrote: post #128
saagarjha wrote: post #118

Earlier quoted context omitted.

Short circuiting boolean operators are very useful, and not something I would consider a code smell. They let constructs like this work, which are very common in most codebases: if (foo && foo->bar) { // whatever } An operator that did not short circuit would have undefined behavior because it might dereference a NULL pointer.

If you're going for "each line of code should do exactly one thing", you'd probably prefer that as if(foo) { if(foo->bar) { //whatever } } separating out the null check and the actual conditional. More lines of code and more nesting, yes, but, if you're trying to strictly adhere to a one thing/one line principle, you probably don't care. Short-circuit 'or' is a little harder to avoid (if you specifically want the sho…

Yep, I'd prefer that option, despite its wordiness. Obviously this depends a lot on individual preferences, and in this simple example it really doesn't make a lot of difference.

I imagine the compiler spits out pretty similar code either way. It's more just a recognition of my own limitations... I'm a lot less likely to screw up something up, the more explicitly it's written out.

When I was a younger man, I wouldn't have bothered.

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#133

Post by areyousure »

Trollmann wrote: post #26
fouric wrote: post #17

From the article: > The line should read "if (key_data_.has_value() && !key_data_->label().empty()) {" but instead of "&&"—the C++ version of the "AND" operator—the bad update used a single ampersand, breaking the second half of the conditional statement. Someone correct me if I'm wrong, but this seems like it wouldn't be possible in a language that didn't conflate boolean values with bitvectors. Edit: I was wrong[1]…

You can use `and` and its siblings instead of && and similar in C++11. Most people here, and in other boards, will try to convince you that it hurts readability because 'that‘s how we always did it' (read I‘m used to it and don‘t like change).

While it is true that you can use `and` in C++11, that's a bit of an understatement: these keywords have in fact been present since the very first ISO C++ standard C++98!

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#134

Post by anyfoo »

Veserv wrote: post #131
anyfoo wrote: post #79

Earlier quoted context omitted.

You are all very quick to judge. I don't work for Google so I don't know what happened, but consider the possibility that this may have been a freak accident. That piece of code for example might only be on a path that is not activated when testing, or it may be executed but not have the same fatal effect. And before anyone comes along and says "well, every possible path should be tested then": That would be absolute…

There are freak accidents and then there are complete lapses is basic critical functionality that demonstrate a total lack of competence. The article indicates that every person who updated to the latest version could not login. This is a bug that clearly presents itself, every time, in essentially every use case, and is perfectly reproducible. It affects critical functionality that any layperson can recognize is one…

I don't know what you are trying to add. Yes, if testing would have uncovered this and the release was put out without that testing, then someone is incompetent. But my point is exactly that the "egregiously visible failure" as you say may not have been visible at all during testing. You seem to think that by "not possible to test all paths" I mean that "logging in" was not tested. That's not at all what I mean.

I mean that even for basic user flows, there is an astronomically high number of possible states and external inputs such that, taken together, something that worked fine during testing may stop working at some point thereafter, or in the hands of the actual users. Latent bugs are called latent for a reason.

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#135

Post by sellyme »

samtheprogram wrote: post #112
anyfoo wrote: post #66

Earlier quoted context omitted.

Once more I hope this is satire, but as so often on the Internet it is surprisingly hard to tell.

This reminds me of Silicon Valley’s main character preferring tabs to spaces because they use less bytes, despite his own product being for... compression! I personally prefer spaces so that under any circumstances, the code reads the same as the author intended (whether you’re in an editor or viewing a file with a CLI tool). Are we really counting bytes in this day and age?

> so that under any circumstances, the code reads the same as the author intended

I write all my code in notepad.exe with the font Wingdings. I hope that if you ever read any of my code you'll respect my intent with how it's displayed.

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#137

Post by bialpio »

formerly_proven wrote: post #44
secondcoming wrote: post #33

Edit: Someone has explained the issue below I don't understand how that broke anything? The C++ codegen for && and & is the same: bool good(bool a, bool b) { return a && b; } good(bool, bool): mov eax, edi and eax, esi ret bool bad(bool a, bool b) { return a & b; } bad(bool, bool): mov eax, edi and eax, esi ret [0] https://godbolt.org/z/84MMqTehs

The code is: key_data.has_value() && !key_data_->label().empty() key_data is probably something like an std::optional, so short-circuit evaluation guards the dereferencing of the optional. If you write this with a binary and, no short-circuit evaluation can happen, so undefined behavior ensues. I wanna point out that std::optional, in the typical C++ stance, has a "I know what I'm doing" API and a "Humans make mistak…

Except that Google uses C++ with exceptions disabled - IIUC, the throwing code would effectively behave like a call to std::terminate(), I haven't looked at the code to know if it would actually be a preferred behavior in this case.

Source: I'm a Chrome contributor.

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#138

Post by MauranKilom »

anyfoo wrote: post #86
gpderetta wrote: post #73

Earlier quoted context omitted.

the problem is that bitwise & and | are routinely used for their non-shortcircuiting behaviour in boolean expressions, so it is hard for the compiler to flag this as an error. Many linters and static analysis tools do flag them though as these days it is not considered good practice.

Why would you even need non-shortcircuiting behavior in boolean expressions? Because either side of the operator has side effects that you want to happen unconditionally? Please just write it out as an extra statement then instead of hammering it into place with an implicit coercion to an integer type only so you can abuse bitwise AND and OR only to force that side effect to... you can see why this is probably not th…

What I would like to write in unit tests:

    for (auto x : y)
      ok &= check(x);
That gets you angry messages from static analysis, so you probably have to write (since there is no corresponding assignment operator)

    for (auto x : y)
      ok = ok && check(x);
...except now you don't actually perform checks after the first failure. That may or may not be intentional (or even confusing - depending on logging). What you'd really need to write to preserve the original logic is

    for (auto x : y)
      ok = check(x) && ok;
Now the `&& ok` is much easier to miss (both in writing and reading) and the intent is much less clear.

I really do wonder why the standard doesn't just specify the behavior of bool & bool. Is it just because of holdover from C?

Re: Google broke a conditional statement that verifies passwords on Chrome OS

#139

Post by khazhoux »

dataflow wrote: post #27
CobrastanJorji wrote: post #21

Earlier quoted context omitted.

Forgive my poor C++, but has_value() sounds like a bool, and empty() sounds like a bool. Why wouldn't "bool1 & bool2" work correctly?

Dereferencing via key_label_-> has undefined behavior when !key_label_.has_value(), so the compiler treats that case the same as if key_label_.has_value(). Or to put it another way, the compiler reasons that key_label_ must have had a value (since you dereferenced it), and thus optimizes out the check for `has_value()` (since clearly it was unnecessary).

I don't think that's right. It seems far-fetched that the compiler would know that accessing key_data->label() implies that k_d->has_value() == true

I believe the explanation is:

   A && B : compiler will evaluate B only if A==true

   A & B : compiler will evaluate both A (safe) and B (unsafe), then perform bitwise operation
Post reply on HN