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

71–80 of 276 posts

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

#71

Post by kps »

sneak wrote: post #10
Traster wrote: post #8

I bet the post-mortem for this is going to be fun. How this wasn't covered by multiple unit tests, a code review and a roll out strategy is.... impressive.

Bitwise & versus logical && is a classic, right up there with an assignment in a comparison (when an equality check is intended), = for ==. That this was missed is pretty surprising, given that it's Google and the stakes involved in the encryption/key management code in a secure platform device. I wonder if we'll even get a postmortem, as this simply cannot happen unless several someones all Seriously Fucked Up simul…

I'm quite surprised there isn't a compiler warning for a bitwise operator at the root of a condition. I've been writing `if ((flags & FLAG) != 0)` for decades years for nothing?!?

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

#72

Post by anyfoo »

dataflow wrote: post #64
myko wrote: post #53

Earlier quoted context omitted.

Would this optimization not have been on the beta/canary channel? I assumed they were but perhaps not since this was not caught.

I don't know how their builds work or where their templates are instantiated but it's possible that the compiler doesn't see this optimization until LTO and that they don't do LTO until stable releases? It would seem strange though, I'd have thought beta would have LTO.

Yeah that would be a very weird choice. LTO itself can have bugs, or (as we speculate here) unmask existing bugs. Plus, even that aside, you'd hopefully test exactly the build that you will release before you release it.

As I wrote in another comment, this could still be the result of a freak accident and not have shown up in testing, though (but since I know nothing about the process or the code there, it could be anything).

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

#73

Post by gpderetta »

dataflow wrote: post #43
fouric wrote: post #31

Earlier quoted context omitted.

Ahhh, so it was a problem with lazy evaluation (and the lack thereof when using &), and not type coercion, like I initially thought.

Yeah it has nothing to do with type coercion. I'm baffled a linter doesn't catch this though. & for bools should just be prohibited altogether. If people really want & for bool, they can just cast to an integer type and use & with that (which generates its own warnings appropriately).

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.

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

#74

Post by shawnz »

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]…

Other than this particular edge case it's not obvious to me why bitwise operators shouldn't be applicable to boolean values. Aren't they just single-bit vectors?

Of course it makes sense that logical operators wouldn't be applicable to numeric types, but why the reverse?

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

#75

Post by shawnz »

grishka wrote: post #52
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]…

Certainly won't compile in Java.

In fact you can apply the single ampersand & operator to booleans in Java, and just like in the example here it has the same behaviour as && but with no short-circuiting.

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

#76

Post by xmprt »

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).

So instead of short circuiting like that code should when it uses &&, it instead makes an assumption about the first part of the boolean expression because otherwise the code would have undefined behavior?

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

#77

Post by aeturnum »

mdoms wrote: post #18
sneak wrote: post #10

Earlier quoted context omitted.

Bitwise & versus logical && is a classic, right up there with an assignment in a comparison (when an equality check is intended), = for ==. That this was missed is pretty surprising, given that it's Google and the stakes involved in the encryption/key management code in a secure platform device. I wonder if we'll even get a postmortem, as this simply cannot happen unless several someones all Seriously Fucked Up simul…

> Heads should probably roll here, I disagree. It's obviously a major balls up by multiple people, but at the end of the day this is an organizational failure. You don't fire people for this, you learn from it and fix the organizational holes that caused it.

I don't think we know enough from this to say.

It's possible that this bug made it into the wild because the organizational structure made remedies impossible. In that case you probably don't fire anyone, though maybe you reassign some folks.

It's also possible that the organizational structure indicates that a person or a group of people is responsible for this and they failed to do that job. If that's the case then it might very well make sense to fire folks. It also might not.

I think it's important for poor performance to have consequences[1] and I also think that people leave jobs all the time and it's fine to decide that someone should leave their place in the org with a positive reference and look elsewhere for work (perhaps in another part of goog).

[1] for moral and company culture reasons if nothing else.

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

#78

Post by dataflow »

gpderetta wrote: post #73
dataflow wrote: post #43

Earlier quoted context omitted.

Yeah it has nothing to do with type coercion. I'm baffled a linter doesn't catch this though. & for bools should just be prohibited altogether. If people really want & for bool, they can just cast to an integer type and use & with that (which generates its own warnings appropriately).

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.

> & and | are routinely used for their non-shortcircuiting behaviour in boolean expressions

Really? I can't recall a single case of this with 'bool' in any codebase in my recent memory. I only see it with integers. And even if it's somehow routine for your codebases with bool (why?!), surely it's not routine inside a conditional expression, so at least they can prohibit it there?

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

#79

Post by anyfoo »

sneak wrote: post #41
idiotsecant wrote: post #35

Earlier quoted context omitted.

>extreme negligence If this doesn't qualify, what does?

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 absolutely impossible even if the halting problem wasn't a thing (which it very much is). Even if you somehow have utopian "100% code coverage", that does not mean you have tested every possible input resulting in every possible state combination.

I bet every one of us has a story about an old bug that only manifested years[1] later because of the confluence of many unfortunate things, one that takes half an hour to tell.

[1] Or even decades, in some cases that then made it to the HN front page.

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

#80

Post by taeric »

grishka wrote: post #52
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]…

Certainly won't compile in Java.

Oddly... It would? Try:

    Map map = ...
    if (map.containsKey("hello") & map.get("hello")) { ...
Post reply on HN