6 ms·

So, Google: 1) included code that performed dangerous system-level operations, despite the fact that 2) the included code was guaranteed to fail under the exp

So, Google: 1) included code that performed dangerous system-level operations, despite the fact that 2) the included code was guaranteed to fail under the expected / default OS configuration? The only logic I could see for such a state would be "let's teach them a lesson" for those users who chose to operate in the "dangerous" configuration. It doesn't make sense why they'd even attempt the symlink removal.

Tuna-Fish 7y ago
or 3) They intended to remove a more specific symlink, with the address generated from variables, but due to bugs some/all of the variables were empty and the concatenation of the variables just produced /var instead. Which is the cause of like 90% of accidental "rm -rf /"s in history.
- TimTheTinker 7y ago
  Right, but if you're a large software distributer, the onus is entirely on you to QA software releases on every possible OS configuration. If this were an individual working alone (or even a small company), I'd have some sympathy. But Google has enough to pay QA engineers and build a sufficiently sophisticated test lab. They should get no pass for this.
  - hinkley 7y ago
    Well, hold on. Let's not scapegoat another team for the fact that we can't seem to handle relative paths well after 30 years of spectacular case studies in how fucking stupid we are with path calculations. I'm always finding people doing string arithmetic instead of using the APIs. Sometimes I catch myself doing it. Rails got close to a solution by half-assedly tracking the provenance of all strings passed into certain functions. We could probably use a bit more of that.
    - TimTheTinker 7y ago
      The fact that this got through Google's QA is inexcusable. Yeah I understand a dev messed up. It happens. But it should have been caught before being released. All it would have taken is testing the installer on a non-SIP Mac.
      - hinkley 7y ago
        In this case a smoke test on such a Mac could have sufficed, yes. But even when I have a QA team, which is less and less often, I like the devs to be involved in setting up stuff like this. QA is often not so good at engineering robust tests. But if they could write better code than we can they’d make more money as devs. So I’m still not letting the devs off the hook. This class of error comes down to our chronic insistence on using stringly typed APIs for path/URI manipulation. It’s tantamount to a SQL injection attack, but with less data in the “query” instead of more. You should not be assembling file paths for destructive writes or deletes by string concatenation. But this is on almost nobody’s radar. Is anyone else in this thread advocating for a change in software design? Mostly we are all blaming Google for fucking up. This is the headspace everyone but PHP was in for SQL fifteen years ago.
        TimTheTinker 7y ago
        > This class of error comes down to our chronic insistence on using stringly typed APIs for path/URI manipulation. Like most failures, the analysis needs to identify a chain of events involving various failures; perhaps it would go something like this: 1. Industry and libraries commonly use strings for path/URI manipulation. 2. A software engineer did so in the installer and made a typo. 3. Code review did not identify the problem. 4. QA (and the CI process) didn't test on a Mac that was either old enough not to have SIP or had SIP disabled. 5. Many Mac users, especially in specialized environments running custom or specialized kernel extensions and drivers, use macOS with SIP disabled. 6. Chrome is widespread enough that some users in (5) downloaded the update. That subset had their Mac systems hosed. I would identify (2), (3), and (4) as problems in the chain where Google carries blame.
- cesarb 7y ago
  Another possibility is that it had a list of all the files and directories it has installed, and removed each one before installing a new version. If the list included also all the parent directories (which is common if you want to avoid leaving empty directories behind), and the list did not specify whether each entry is a file or a directory (that is, it looked at the disk for each entry: if it was a directory, it did a rmdir, otherwise it did an unlink), and it installed anything below /var, all that combined plus the fact that /var was a symlink instead of a directory could lead to it calling unlink on /var, while the developers would be expecting it to do a harmless rmdir (which does nothing unless it's an empty directory).
aeorgnoieang 7y ago
No, [2] is incorrect – exactly opposite. The included code was 'guaranteed to' (and did) work under the default OS configuration.
- hinkley 7y ago
  Like kids' bowling, where they block up the gutters.

Tuna-Fish 7y ago

or 3) They intended to remove a more specific symlink, with the address generated from variables, but due to bugs some/all of the variables were empty and the concatenation of the variables just produced /var instead. Which is the cause of like 90% of accidental "rm -rf /"s in history.

TimTheTinker 7y ago
Right, but if you're a large software distributer, the onus is entirely on you to QA software releases on every possible OS configuration. If this were an individual working alone (or even a small company), I'd have some sympathy. But Google has enough to pay QA engineers and build a sufficiently sophisticated test lab. They should get no pass for this.
- hinkley 7y ago
  Well, hold on. Let's not scapegoat another team for the fact that we can't seem to handle relative paths well after 30 years of spectacular case studies in how fucking stupid we are with path calculations. I'm always finding people doing string arithmetic instead of using the APIs. Sometimes I catch myself doing it. Rails got close to a solution by half-assedly tracking the provenance of all strings passed into certain functions. We could probably use a bit more of that.
  - TimTheTinker 7y ago
    The fact that this got through Google's QA is inexcusable. Yeah I understand a dev messed up. It happens. But it should have been caught before being released. All it would have taken is testing the installer on a non-SIP Mac.
    - hinkley 7y ago
      In this case a smoke test on such a Mac could have sufficed, yes. But even when I have a QA team, which is less and less often, I like the devs to be involved in setting up stuff like this. QA is often not so good at engineering robust tests. But if they could write better code than we can they’d make more money as devs. So I’m still not letting the devs off the hook. This class of error comes down to our chronic insistence on using stringly typed APIs for path/URI manipulation. It’s tantamount to a SQL injection attack, but with less data in the “query” instead of more. You should not be assembling file paths for destructive writes or deletes by string concatenation. But this is on almost nobody’s radar. Is anyone else in this thread advocating for a change in software design? Mostly we are all blaming Google for fucking up. This is the headspace everyone but PHP was in for SQL fifteen years ago.
      - TimTheTinker 7y ago
        > This class of error comes down to our chronic insistence on using stringly typed APIs for path/URI manipulation. Like most failures, the analysis needs to identify a chain of events involving various failures; perhaps it would go something like this: 1. Industry and libraries commonly use strings for path/URI manipulation. 2. A software engineer did so in the installer and made a typo. 3. Code review did not identify the problem. 4. QA (and the CI process) didn't test on a Mac that was either old enough not to have SIP or had SIP disabled. 5. Many Mac users, especially in specialized environments running custom or specialized kernel extensions and drivers, use macOS with SIP disabled. 6. Chrome is widespread enough that some users in (5) downloaded the update. That subset had their Mac systems hosed. I would identify (2), (3), and (4) as problems in the chain where Google carries blame.
cesarb 7y ago
Another possibility is that it had a list of all the files and directories it has installed, and removed each one before installing a new version. If the list included also all the parent directories (which is common if you want to avoid leaving empty directories behind), and the list did not specify whether each entry is a file or a directory (that is, it looked at the disk for each entry: if it was a directory, it did a rmdir, otherwise it did an unlink), and it installed anything below /var, all that combined plus the fact that /var was a symlink instead of a directory could lead to it calling unlink on /var, while the developers would be expecting it to do a harmless rmdir (which does nothing unless it's an empty directory).

aeorgnoieang 7y ago

No, [2] is incorrect – exactly opposite. The included code was 'guaranteed to' (and did) work under the default OS configuration.

hinkley 7y ago
Like kids' bowling, where they block up the gutters.