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

81–90 of 276 posts

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

#81

Post by kbelder »

fouric wrote: post #31
dataflow wrote: post #27

Earlier quoted context omitted.

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

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

I think this is a unpopular opinion, but I consider taking advantage of short-circuiting in boolean operations to be a code smell. At least for me, it seems very fragile and I'd much rather break the conditional logic out explicitly.

But I like to write as much like assembly as I can in every language. Each line of code should do one and only one thing.

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

#82

Post by kevincox »

Animats wrote: post #19

The reason this happened is that it wasn't followed by "Google hit by 143 defective product lawsuits".

I don't think they could possibly be sued for this. They would just be responsible for fixing or refunding the product. In this case they issued a "fix".

It is the same as when a car is defective. They don't get sued, they do a recall and fix it.

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

#83

Post by dataflow »

xmprt wrote: post #76
dataflow wrote: post #27

Earlier quoted context omitted.

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?

Yeah. More specifically, it just inserts whatever behavior it likes for the undefined scenario, which in this case is "the same behavior as in the defined scenario".

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

#84

Post by dataflow »

kbelder wrote: post #81
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.

I think this is a unpopular opinion, but I consider taking advantage of short-circuiting in boolean operations to be a code smell. At least for me, it seems very fragile and I'd much rather break the conditional logic out explicitly. But I like to write as much like assembly as I can in every language. Each line of code should do one and only one thing.

I used to only put short-circuits when I really needed short-circuit behavior, thinking it improved readability. But after getting by the implicit type conversions over and over (as well as nags from every compiler) I just decided it was a bad idea in C++. I'd still do it in a safer language, but in C++ I'm convinced you should just use && even when you only need & for bools.

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

#85

Post by vlovich123 »

idiotsecant wrote: post #35
nicoburns wrote: post #24

Earlier quoted context omitted.

> By that logic, almost no individual failure is a firing offence I'd probably agree with that. Firing should be for extreme negligence, or sustained underperformance.

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

This isn’t extreme negligence of any single individual. If there are linters/compiler warnings that should have caught this, this is an organizational failure that can be remedied and the manager/TL should be taking a bit of a hit here as having those enabled is best practices. Since it’s ChromeOS I imagine there’s a dedicated team managing the build infra and it’s not using the base set of things that Google3 protects you with, so the fault may lie in that organization a bit as an oversight.

Then there’s obviously the automated testing strategy, manual testing strategy and sanitizer strategy that missed this.

The net result is extreme negligence but to me this kind of failure speaks to several layers going wrong at once rather than one person fucking up badly. Organizationally you try to defend against these kinds of failures with multiple layers of defenses.

Now if this was a malicious internal attack that should be investigated but I’d hate to have to seriously reprimand anyone who typo’ed & instead of &&.

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

#86

Post by anyfoo »

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.

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 the most elegant idea.

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

#87

Post by Guidii »

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…

"Heads should probably roll here..."

Google has a blameless post-mortem process (see https://sre.google/sre-book/postmortem-culture/) which essentially boils down to:

0) We want to fix the situation. By the time the post-mortem happens, that should already be done.

1) We want to make sure this never happens again. In order to do that we need to understand as much as we can about what happened.

2) We want everyone to share everything they know about the event. If the participants worry about fallout, they will be compelled to cover up.

3) Most failures happen at a number of points, so you need to dig in deeply if you want to get full value out of the "event".

4) Good post-mortems lead to process fixes to reduce (or eliminate?) the chance of similar events, or to make fixes throughout the code base.

The other thing is that after the post-mortem, all of the folks involved have learned from it. Usually they're better engineers because of that expensive lesson you've just paid for.

Disclaimer: Googler, participant in several post-mortems, opinions my own.

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

#89

Post by agilob »

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…

RedHat once committed wrong value of MTWO variable, it was 2, there was another variable called TWO which also had value of 2. It was fixed a few months later with a commit like "change value of MTWO to -2".Sorry, but can't find it find now

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

#90

Post by crazygringo »

anyfoo wrote: post #50
nogridbag wrote: post #29

Earlier quoted context omitted.

I've seen the = instead of == in the wild with disastrous results: the assignment was on an ORM backed object, e.g. // accidentally mutates all names to Sam and persists to the DB myList.filter { myDomainObj.name = 'Sam' } But it makes me wonder why our programming languages would use characters which can often lead to this type of error.

The problem is not even with "easy to mix up operators" for this, it's about languages without a strong and static enough type system. While the particular problem Google hit is a bit more subtle (involving undefined behavior), in the majority of cases a strongly typed language would not allow expressions that accidentally mix up = and ==, as they often resolve to different types. In C "if (foo = bar)" and "if (foo =…

> in the majority of cases a strongly typed language would not allow expressions that accidentally mix up = and ==, as they resolve to different types.

That's not true at all.

Whenever you're intending to compare equality, they're usually going to be the same type already unless you're being really slopping in a scripting language like JavaScript or PHP -- but even then they're usually two numbers, two strings, or two booleans.

The whole problem here is where foo and bar are both integers, or booleans, or whatever you want, and instead of typing "if (foo == bar)" you type "if (foo = bar)".

Strong typing does nothing whatsoever to help in this common situation.

Post reply on HN