Hacker News Reader

Comment by sneak | Hacker News Reader

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

- code error (expected, humans are fallible)

- review error (less expected, this is the kind of thing code review exists for)

- static analysis / linting check failure

- integration test failure (interactive login no longer works on this build)

This is a massive, overlapping fuckup. Heads should probably roll here, especially given that when you opt in to ChromeOS as a user, Google now has "one job" (DFIU).

View on HN

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

Everyone should support blameless postmortems. I'm more ruing the impending "oh we're soooo sorry" PR babble where it reads as a subtle "** off pleb".

Blameless post-mortems are good, but there have been rather a lot of bootloader level disasters on ChromeOS/Android and where they intersect at this point and it is probably worth wondering why this process does not seem to be generating sufficient improvement to make customer devices work better.

One notes that even years later Google still haven't figured out why the Pixel C occasionally just forgot it's own passcode and encryption key and locked users out. Thousands of reports, and publicly acknowledged as an issue. Never addressed. Pixel devices have had multiple bootloader issues and are currently experiencing a widespread Widevine downgrade for months and not fixed.

Sounds nice, but how much is this really true in practice? If one guy on the team seriously screws up, won't the rest of the team naturally trust him less?

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.

If you consider a linter part of a programming language, most, when properly configured, do not permit this. It's just the syntax that does; professionals do not rely on syntax validity alone but employ linting, code review, static analysis, and integrations tests on top.

The gradient of professionalism in our industry is very wide, and the slope is quite shallow.

To your point, however, many languages (such as Go, developed and used by Google, the organization under discussion) have designed their syntax now to completely avoid this type of error even being possible.

> type of error even being possible

I mean its still possible to forget the ":" character and its still possible to mentally scan a PR and see "=" and miss that it should have been a "==".

= vs == isn't possible in Go because of the statement/expression distinction. The benefits of allowing assignment to be an expression are too small compared to the problems it causes.

  x == 1 // oops, meant =
  // x == 1 evaluated but not used

  if x = 1 { ... } // oops, meant ==
  // syntax error: assignment x = 1 used as value

Ah I see your point. Appreciate the correction.

Python had similar, then introduced := for explicit assignment-as-expression. The debate over that one is considered a contributing factor to Guido's retirement as bdfl

what is the difference between linting and static analysis? Coverity, cppcheck etc, what are those? You mention integration tests, but not unit test, do you value unit test?

You could consider linting a subset of static analysis that considers only smaller units of code and not program flow. A linter might tell you your variable names are wrong or your brackets are wrong or you used discouraged method xyz. A different static analysis tool might tell you there's a path through a function that leaks memory.

I'm a fan of programming languages using := for assignment for this reason

Look, I know that this is an enormously bad bug, but...

Google has a codebase that numbers multiple billions of lines. Using := means typing multiple billions of extra characters. That adds up. And it wouldn't even have prevented this bug!

All checks have costs. As Emerson said, "A stitch in time saves nine. So we make 1000 stitches, that by doing so we may save nine."

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?

The argument for tabs really goes that time is spent much more on reading code than authoring, so it should be optimized and customizable (tab width) for the reader.

Tabs for indentation, spaces for alignment.

I used to feel very strongly about this. Then I started using Common Lisp and discovered the abomination that is tabstop. This archaic "feature" leads to tab width that varies across the line and seems to be hardcoded in every text editor out there. Worse, it's only useful if using tabs for alignment which is completely broken anyway! At some point I intend to patch the source for the editors I use to remove this nonsense.

Admittedly this is something of an edge case - how many languages other than Lisps involve alternating layers of indentation and alignment? The more common C like languages don't suffer from this at all.

At this point I'm largely convinced that more or less all of our tools, languages, and conventions are poorly thought out, brittle, and inelegant. /rant

usually I turn off tabs for indentation in Lisp editors. On a Lisp Machine:

  (setq zwei:*indent-with-tabs* nil)

I've settled on "follow the existing/upstream project's style; if it's a new project, follow the language's idiomatic style". I personally (strongly) prefer spaces over tabs, but I've inherited/forked plenty of projects that use tabs in JavaScript for example, and I just set my editor and formatter to use tabs in that project.

I think the ideal situation is a language with an official or at least commonly accepted formatter, so that existing and new projects will essentially always follow the same style and you never have to think about it. Like "go fmt" always using tabs. I don't like tabs, but I like that kind of enforced consistency way more than I dislike tabs.

That's also why I like using opinionated third-party formatters like prettier and black. Combined with pre-commit hooks, you almost never have to think about style and can just focus on the code. Plus diffs are cleaner.

Considering that most code get read by more than one person, I think hardcoding the author's preferences in how it should be read is strange and in some cases inaccessible.

This. Author chose two spaces per indent and proceeded to nest blocks 5 levels deep? Guess I have to run it through a code formatter now just to read it.

Alternatively, author chose eight spaces per indent and proceeded to nest things? My tiled windows only have ~100 columns, thanks so much for breaking my workflow with your poor choices.

But why not? These days reformatting on the fly is fast and easy. Especially if you enforce formatting conventions on commit (so you can automatically reformat into a canonical version).

Because typically code will be read far more than it is written and by far more people than contributed to it. In many cases I won't have the relevant tooling installed let alone know how to use it because I don't use that language myself. (For example, a library written in Rust that exposes C bindings whose prebuilt artifact I might download and link against.)

That being said, I 100% wholeheartedly approve of enforcing code formatting (among other things) on commit. Pre-commit hooks and linters are both awesome.

If you do not have the relevant tooling you can still read the canonical formatting.

If you want custom formatting, I don't think that expecting your editor to reformat on the fly is a big ask. At the end of the day that exactly what tab proponents expect.

> If you do not have the relevant tooling you can still read the canonical formatting.

Which is often unnecessarily difficult for whatever reason, hence my initial comment. Given that tabs already exist there's literally no reason not to use them other than to intentionally spite any future readers who don't agree with your preference for indentation width.

> that exactly what tab proponents expect

Because that capability is built into even the most primitive of text editors since forever. Per-language formatting, on the other hand, is most certainly not.

Using just tabs you end up with misaligned code. So you need a mix of tabs and spaces (i.e. smart tabs). But nobody sane [1] would do that by hand and would rely on their editor to do the indenting automatically. At this point just ask the editor to indent on demand.

[1] I'm hyperbolic, sorry if you actually do that although I pity you.

Automated reformats cause big problems in version control.

Every line appears to come from the reformat commit. So it's hard to see when the codes function was changed.

you format as part of the commit, not as a separate commit.

I believe what's being described is on the fly (ie uncommited) reformatting to suit individual reading preferences. Additional enforced autoformatting at the time of commit ensures that all version controlled code always conforms to the chosen style. Thus there are never any reformat commits unless the chosen style is modified.

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

I prefer <- myself :)

Another Smalltalk fan!

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 == bar)" are both legal, because pretty much any type can be automatically coerced into a boolean. In many modern languages that is not the case. (Of course if you have assignments as expressions you are still screwed if foo and bar are both boolean to begin with, so some languages got away with that, too. And for fairness I should say that C at the very least warns if you don't explicitly put parentheses around the assignment, nowadays.)

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

No. It can help because the result of the expression would have a different type, one that "if" does not accept. If a and b are integers, then the expression "a = b" is an integer too, but "a == b" is a boolean. In a strongly typed language, you can assume that "if" only accepts booleans.

Yeah. The direction of modern languages has been away from features like this which are "clever" but don't express anything you couldn't have said by another means - and yet allow programmers to accidentally write something nobody would ever want.

Assignment shouldn't have a return type. Compound assignment operators? Likewise. You never need this return value, and sometimes you may use it by mistake. So, abolish it. In C++ not only do operators that shouldn't have a return value have one anyway, the type of that value might be anything. You can even overload both ++ operators to return different unrelated types so that

  mytype aThing { blah };
  auto a = aThing++;  // returns a String "Fuck tha police"
  auto b = ++aThing;  // returns a double, 8.63

The Jeff Goldblum GIF is often the appropriate reaction to C++ features:

"Your scientists were so preoccupied with whether or not they could, that they didn't stop to think if they should"

In many languages, such as python, assignments are statements, not expressions, and so they don’t evaluate to anything at all.

Yeah, I covered that at the end.

>But it makes me wonder why our programming languages would use characters which can often lead to this type of error.

Guido van Rossum wondered the same, which is why it isn't possible in Python. Same for "&&" vs. "&", since in Python it's "and" vs. "&".

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

Someone is responsible for the organizational units and their processes.

By that logic, almost no individual failure is a firing offense. Many people in the organization are responsible for ensuring that the processes themselves overlap in ways that exclude the possibility of failures like this.

I'm talking about the meta-failure in management's oversight of the processes. There is belt person, and there is suspenders person, and there is a third person who makes sure there are both belt person and suspenders person on staff and that they don't go on vacation at the same time.

Somewhere above the team lead and below the CEO there is someone who didn't do their job.

> 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?

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.

This is the sort of thing that most linting tools will catch, so it's a bit worse than that I think. You shouldn't need any test coverage to catch this.

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.

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.

One point that doesn't seem to have been explained yet - mentioned in the OP article - is that Chrome OS has canary, dev, and beta channels prior to stable, with real users.

For something like this to have happened without those non-stable channel users experiencing it, it seems like there must have been a code change that was pushed direct to the stable channel - which negates the purpose of the other channels in the first place.

At the very least it seems like a severe process failure. I don't think the dev who changed the code is necessarily at fault - it's more a question of how it was allowed to get into the stable channel without going through the normal process.

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

The GGGP is talking about failures at a higher level. Not reprimanding the coder who missed an ampersand, but the manager who was ultimately responsibly for ensuring there was a functioning process for catching this sort of nonsense.

Reprimanding is one thing and firing another though. I certainly think the manager in charge deserves a reprimand.

I wouldn't even say a reprimand is necessary here.

Programming safety is like safety in auto racing or rock climbing: you build the safest thing you can, but the activity you're doing is inherently unsafe, and it will generate failures despite the presence safety measures, so you do your best to study those failures to mitigate them the best you can.

The only time adverse action like a reprimand is necessary is if the manager or tech lead don't take any action to prevent future issues. The only time a firing is necessary is if the security issue was created with intent, rather than accidentally.

This specific type of failure is evidence that they didn't build the safest thing they can.

Linting catches this.

In situations like these there's not really one manager, so who are you reprimanding? I'm sure there's failures all around. Seriously, blameless post mortems are a cultural thing you need to adopt. Reprimanding just instills fear which causes people to take actions that you don't want organizationally (shifting blame/responsibility, hiding issues, politicking etc). Better for people to just be proactive about when a problem exists & marshal your org to prevent such issues in the future. It's unlikely to be the cause of 1 individuals incompetence so any individual(s) you find to blame are just victims of circumstance.

Where you do want to reprimand is if the technical team was repeatedly raising concerns related to failures like this & their concerns weren't being addressed OR someone was being deceitful & hiding issues. That's about the only cases where you want to fire/replace anyone in my book.

The way you fix organizational/process failures is not by firing the person in charge of them every time they fail to catch something.

How many breaking bugs have _NOT_ shipped due to the current processes. Your approach suggests that there is some perfect process manager out there who will let no bugs like this ship (whilst not dropping any other business priorities mind you) that they just haven't hired yet into this position.

This isn't as big of a bug as people on HN want it to be. A few months from few people will remember, and even fewer will care.

> By that logic, almost no individual failure is a firing offense.

That's pretty close to how it should be. That's the core premise of the no-blame post-mortem culture that's great about some companies.

As long as you go through the port-mortem process, which includes doing your best to prevent this from happening again (which may include changing organizational processes, testing strategies, etc), then everyone can move on with their lives.

Firing happens when you have a history of performing below your level. Never for a single engineering failure.

This isn't an engineering failure. Everyone writes bugs.

Engineering process is still engineering in my book, but if it helps:

Firing happens when you have a history of performing below your level. Never for a single process failure.

There is a story (perhaps apocryphal) of a salesman who lost a $10 million deal for IBM. He was sure he was going to be fired. Some bigwig (Watson himself, maybe?) talked him through what had gone wrong. The salesman asked, "Aren't you going to fire me?" The bigwig said, "I just spent 10 million dollars training you."

> By that logic, almost no individual failure is a firing offense.

Correct.

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.

Especially since it's so easy to lint for. This isn't some cross-file memory mishandling bug, it's a single token that's being used where it shouldn't be

> this simply cannot happen unless several someones all Seriously Fucked Up simultaneously.

This is exactly how any and every bug enters a mature software product: it slips past multiple layers.

> This is a massive, overlapping fuckup. Heads should probably roll here

Some process should change, to fix the problem. But firing people is not necessarily the best solution.

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

Perhaps they should have deleted that variable…

While externalizing repeated constants can be useful (e.g. defining PI_4 for a math routine that uses pi / 4 a dozen times), taken to an extreme like that it leads to more bugs, not fewer.

glances at screen LGTM

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?!?

Hmm. In all seriousness, you could probably build a lint rule/tool to allow only the format you note.

Straight up negligence.

Reply on news.ycombinator.com