5 ms·

The undocumented Android change that led to aCropalypse was reported during beta

337 points by luu 4y ago

justin_oaks 4y ago
Another case of poorly chosen defaults leading to bugs, leading to security issues. Also a case of violating the Principle of Least Astonishment [0]. How many people would expect that when you write a file you would also have specify that you want it truncated? [0] https://en.wikipedia.org/wiki/Principle_of_least_astonishment https://en.wikipedia.org/wiki/Principle_of_least_astonishmen...
- jandrese 4y ago
  It's notable that in practically every other programming language on every platform the default is to truncate the file to 0 bytes when opened in write (not append) mode. This isn't one of those things where there are two schools of thought where people take sides. It's a completely baffling decision to buck all convention vs. common sense.
  - Someone 4y ago
    > It's notable that in practically every other programming language on every platform the default is to truncate the file to 0 bytes when opened in write (not append) mode Examples of ‘Open’ calls that require setting a bit in the ‘flags’ argument to truncate an existing file: - https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-openfile https://learn.microsoft.com/en-us/windows/win32/api/winbase/... - https://man.freebsd.org/cgi/man.cgi?query=open&sektion=2&n=1 https://man.freebsd.org/cgi/man.cgi?query=open&sektion=2&n=1
    - okanat 4y ago
      You're comparing system call interfaces to higher level standard API defaults. Not really an apples to apples comparison. System calls are special low-level interfaces. The requirement for reading their docs carefully and general care is expected at that level. A higher level API shouldn't contain gotchas like this.
    - moefh 4y ago
      Wow, that Win32 OpenFile function is truly cursed: - If you don't specify a directory, Windows will look for the file in a bunch of directories, but the current directory is not the first place searched. - The lpReOpenBuff is annotated as [out], but is an input when uStyle contains the OF_REOPEN flag. - The function is named OpenFile, but can be used to delete a file. Even better, the documentation is not clear what to do with the return value on success: is it a valid file handle that must be passed to CloseHandle, or just a dummy non-error file handle? - You can also use it to check the existence of the file -- the documentation says it will open and then close the file. What's the return value in that case? Is it a stale file handle, a dummy, or do you still have to close it?
      - Someone 4y ago
        You’re forgetting this one: you can specify a flag that makes the call display a dialog box if a requested file does not exist. Yes, this call to open a file can run an event loop that lets the user create directories, rename files, possibly even format floppy disks, etc. If you want a prime example of a layer violation, this is it. Its replacement “CreateFile” (https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea https://learn.microsoft.com/en-us/windows/win32/api/fileapi/...) is a lot saner, but still has this: “If you rename or delete a file and then restore it shortly afterward, the system searches the cache for file information to restore. Cached information includes its short/long name pair and creation time.” That’s intended to make it easier for “Save” operations in document-based applications to keep the short name of the file, creation time and file flags around.
- deleted 4y ago
  [deleted]
- intelVISA 4y ago
  Some say certain agencies wanted it this way.
  - jmull 4y ago
    Some are yet naive. “Never attribute to malice that which is adequately explained by stupidity.”
    - account42 4y ago
      Are you trying to volunteer yourself as an example of naivety.
  - bagels 4y ago
    Who says this?
    - dylan604 4y ago
      They and them. Don't confuse the pronouns as being more than one person.
  - rozab 4y ago
    This would be the stupidest possible way of conducting mass surveillance
amethyst 4y ago
Unrelated: the nostalagia of iliana's favicon is incredible. So much time spent on old modems watching that animation spin while waiting for pages to slowly load in...
- neilv 4y ago
  That one was great, and stylish. The original NCSA Mosaic one with the globe could've been a bit much, but it was appropriate, given the impact of the Web, even for pre-Web Internet natives: "OMG, this is a tingly step forward in distributed global hypertext, it's happening right now -- interacting around the world, and also superpowering the world." With implied altruistic and benevolent goodness. (The earlier Mcom/Netscape throbbing "N", when I guess they were still rebranding Navigator from Mosaic, looked silly even at the time. Then they did a good one. And there were a bunch of creative alternatives, including some easter egg ones, which JWZ cataloged at one time.)
- hu3 4y ago
  It's an animated gif! https://iliana.fyi/netscape.gif https://iliana.fyi/netscape.gif
canucker2016 4y ago
Mentioned in the link. This seems to be the original PR for the API in question, ParcelFileDescriptor#parseMode(): https://android.googlesource.com/platform/frameworks/base/+/eb8c3f93edc826413ff4143284dec01c1061d5cc%5E%21/#F3 https://android.googlesource.com/platform/frameworks/base/+/... + public static int parseMode(String mode) { + final int modeBits; + if ("r".equals(mode)) { + modeBits = ParcelFileDescriptor.MODE_READ_ONLY; + } else if ("w".equals(mode) || "wt".equals(mode)) { + modeBits = ParcelFileDescriptor.MODE_WRITE_ONLY + | ParcelFileDescriptor.MODE_CREATE + | ParcelFileDescriptor.MODE_TRUNCATE; + } else if ("wa".equals(mode)) { ... You can see that "w" and "wt" map to the same mode for file operations, including the MODE_TRUNCATE flag. But we jump ahead a few years and the code is reworked. from https://android.googlesource.com/platform/frameworks/base/+/63280e06fc64672ab36d14f852b13df2274cc328%5E%21/core/java/android/os/ParcelFileDescriptor.java https://android.googlesource.com/platform/frameworks/base/+/...: public static int parseMode(String mode) { - final int modeBits; - if ("r".equals(mode)) { - modeBits = ParcelFileDescriptor.MODE_READ_ONLY; - } else if ("w".equals(mode) || "wt".equals(mode)) { - modeBits = ParcelFileDescriptor.MODE_WRITE_ONLY - | ParcelFileDescriptor.MODE_CREATE - | ParcelFileDescriptor.MODE_TRUNCATE; - } else if ("wa".equals(mode)) { ... - return modeBits; + return FileUtils.translateModePosixToPfd(FileUtils.translateModeStringToPosix(mode)); } So what does FileUtils.translateModePosixToPfd(FileUtils.translateModeStringToPosix(mode)) do? from https://android.googlesource.com/platform/frameworks/base/+/63280e06fc64672ab36d14f852b13df2274cc328%5E%21/#F2 https://android.googlesource.com/platform/frameworks/base/+/...: + public static int translateModeStringToPosix(String mode) { + int res = 0; + if (mode.startsWith("rw")) { + res |= O_RDWR | O_CREAT; + } else if (mode.startsWith("w")) { + res |= O_WRONLY | O_CREAT; + } else if (mode.startsWith("r")) { + res |= O_RDONLY; + } else { + throw new IllegalArgumentException("Bad mode: " + mode); + } + if (mode.indexOf('t') != -1) { + res |= O_TRUNC; + } + if (mode.indexOf('a') != -1) { + res |= O_APPEND; + } + return res; + } Now "w" and "wt" map to different Posix open mode flags since one has to explicitly pass in "t" to get file truncation. A dev was careless, plain and simple. Having code in a monorepo doesn't help if you don't check how your code changes affects consumers/clients of the code. If they couldn't change translateModeStringToPosix() behaviour, then the dev could've amended parseMode() to map "w' to "wt" to keep the old behaviour (ugly and non-orthogonal, yes, but that's the way the API was designed).
TazeTSchnitzel 4y ago
I wonder if some developer of an app working with ZIP files will have noticed this bug and had to update their app. ZIP is an unusually terrible file format because it has a footer rather than a header, so the truncation bug would lead to a ZIP file with two headers!
- canucker2016 4y ago
  Some ZIP-file handling apps go straight to the end of the ZIP file and work backwards looking for the central directory. I believe others will start from the beginning of the file and parse each local directory header and 1) build up their own central directory truth, or 2) stop once they've reached the start of the central directory. I recall that the footer-only format was to enable streaming out a ZIP file esp. when rewinding to the front of the file/stream was not possible, like sending over a network - at least that's one of the use cases I recall.
  - ladberg 4y ago
    I believe the footer-only format was more about being able to make modifications by only appending to the zip file and not having to rewrite/shift any data.
    - canucker2016 4y ago
      ...the central directory needs to be shifted by the amount of the new compressed data + local directory header. But that's a lot less than if the central directory was at the beginning and the code had to grow the central directory and shift ALL the compressed data + local directory headers down.
      - ladberg 4y ago
        Yeah that might be the ideal way to do it, but technically you could just append the new entries and new central directory without even removing the old one. I'm not entirely sure what storage system that would make sense on, but presumably there was one back in the day.
neilv 4y ago
Kudos to the engineer who originally not only identified the problem, but also raised the concern about breaking behavior, and about documenting it. Looks like the ball was first dropped when the issue was deferred without acknowledging the reporter's concise suggestions of how to handle it, engineering-wise. Nor a sign that existing code was reviewed for the same problem. Then looks like the ball was dropped again, in what I'm guessing might've been: "old version, forget all the old open issues, all the issues that still apply, someone will rediscover the hard way". This "aCropalypse" event is an example of how making the wrong triage of an issue report can turn out very expensive. All the costs to the world of aCropalypse could've been averted. Some of those costs might eventually come back to the company. It's easy to guess why the problem happened and the report wasn't handled responsibly. By the time the problem exhibited in a way that couldn't be ignored, the pertinent metrics/KPIs/OKRs about clearing issues, resource allocations, and product shipment schedules were already in the past, bonuses had been paid, promotions for shipping new things earned, etc. And security problems are treated as inevitable, even if there's a constant stream of them, and they're produced faster than they're fixed. We're going to need software engineers to be accountable for things we've done and signed off on. (Bonus if accountability changes happen near-term: GPT-powered open source laundering gets a pause, because bridges start falling on everyone foolish enough to sign off on that mangled statistical plagiarism.)
- yjftsjthsd-h 4y ago
  > We're going to need software engineers to be accountable for things we've done and signed off on. Management first. It's not engineers setting incentives.
  - ryandrake 4y ago
    This is just passing the buck. If the boss tells you to write credit card stealing software or to DDOS your competitor are you just going to do it? As professionals, we need to push back and refuse when we know our practices are letting critical bugs go to production, just like (I assume) we'd push back if we were asked to build something unethical. And we should be accountable if we don't. If a civil engineer signs off on a bridge and it collapses during rush hour, he doesn't get to just say, "Whelp, boss rushed me, blame him."
btown 4y ago
A cautionary tale here: when you are testing, if you mock out even low-level I/O utilities, you’re vulnerable to those utilities changing specification subtly. You can supply-chain attack yourself by simply updating a library. Full filesystem/service-engaging integration tests are helpful but far from exhaustive - here, one wouldn’t have just needed to read the cropped file using standard display systems and count the pixels, but hash the underlying file in its entirety. For critical libraries in an I/O pipeline, encourage team members to read CHANGELOGs - both before choosing a library to make sure they’re maintained, and closely when even doing a minor upgrade.
- DoctorNick 4y ago
  Change logs wouldn’t have helped here, this behavior change was undocumented.
  - btown 4y ago
    IMO the Android team should have realized the Android team was not creating internal libraries at a sufficient level of stability and change tracking for the Android team. This quickly gets into cultural priorities that I have very little context on - but I do think that processes failed here.
jimrandomh 4y ago
It looks like this was reported as a normal bug, and the security implications weren't noticed until later. This is unfortunate, but it's also pretty normal and hard to avoid this sort of thing happnening.
- layer8 4y ago
  It was declared to be an intentional change and not a bug. If anyone would have thought through how an application expecting the old semantics will behave under the new semantics, it should have been rather obvious that silently changing the semantics is a very bad idea. It’s absolutely a case of professional negligence.
sitkack 4y ago
By Design Won't Fix Working As Intended Declaring Bug Bankruptcy Google doesn't test. Google relies on the public to test its software and even then most bugs don't get addressed. If the bug is a gap in the spec or open to interpretation it won't get fixed. Spec bugs are not allowed. The reason that Google has such a high hiring bar is that most of its processes run open-loop and it relies on having smart thorough people to prevent the large swath of issues from being created in the first place.
- lapcat 4y ago
  > The reason that Google has such a high hiring bar is that most of its processes run open-loop and it relies on having smart thorough people to prevent the large swath of issues from being created in the first place. But there is a large swath of issues. Just look at Google's public bug trackers. And Google's atrocious practice of auto-closing old bug reports partially covers up how many issues there really are.
codethief 4y ago
This reminds me of how a recent Android version (12?) suddenly made certain characters (e.g. ?:<>) illegal in file names[0], probably because to increase compatibility with NTFS. Wtf, Google. Nowhere was this breaking change announced. Not only does my notes app, which has worked flawlessly for years, now crash when I accidentally use one of those characters in a file name / note title. (Unfortunately, the app no longer receives updates.) But suddenly my entire ebook library (which follows the name format "first author & second author: title (year)") no longer synchronizes with my phone (running AOSP / GrapheneOS), either. Fantastic. [0]: https://android.googlesource.com/platform/packages/providers/MediaProvider/+/6b7baf4b9722dd75bfb5d64acadf48223226cde6%5E%21/#F0 https://android.googlesource.com/platform/packages/providers...
- dpkirchner 4y ago
  Makes you wonder what issue they were mitigating. Passing file names as part of a concatenated string to bash? Ala 'bash -c "ls $filename"'?
  - kps 4y ago
    Windows. /** * Sanitizes given name by mutating the file name to make it valid for a FAT filesystem. */ https://cs.android.com/android/platform/superproject/+/master:packages/providers/MediaProvider/src/com/android/providers/media/util/FileUtils.java;drc=master;l=1461 https://cs.android.com/android/platform/superproject/+/maste...
    - codethief 4y ago
      Ah, good point. So it was not NTFS but FAT compatibility they were aiming for.
- mikepavone 4y ago
  Oh, I was wondering my new phone didn't like some of the music files I was trying to copy over using MTP even though the old one handled them fine. I had assumed the phone manufacturer did something dumb, but apparently it was Google all along.
OnlyRepliesToBS 4y ago
[dead]
cvccvroomvroom 4y ago
Developers at big companies aren't motivated to do everything perfectly. They're graded for big wins, not tech debt, documentation, or correctness.
JoeAltmaier 4y ago
I have sympathy. It's hard to write fundamental APIs. They need to grow and change like anything else, always at the risk of startling somebody. Years ago I extended a network file system server to include additional information in an Open response (revision tag to enable distributed caching). Turns out a customer had also extended the server response in their builds, to do something else security-related. Our release startled them, resulting in a lawsuit. It was all silly and fixed in a patch (a boot config file enable/disable of our new extended info) and it went away. But it underlines how with the best of intentions, still things go wrong.
jasonhansel 4y ago
Why would cropping alter the file in-place? That would presumably leave the image corrupted if (say) your phone died during the cropping process; it would seem much better to save the cropped image separately and then overwrite the original file atomically. After all, it's the same amount of data to write either way.
- ledauphin 4y ago
  this comment should be higher. atomic move semantics are the single thing that makes writing basic filesystem interactions plausibly deterministic. anything else is just pretending that errors never happen.
lupire 4y ago
> This bug was closed as obsolete a year later. The infamous "Bug Bankruptcy"

337 points by luu 4y ago

Another case of poorly chosen defaults leading to bugs, leading to security issues. Also a case of violating the Principle of Least Astonishment [0]. How many people would expect that when you write a file you would also have specify that you want it truncated? [0] https://en.wikipedia.org/wiki/Principle_of_least_astonishment https://en.wikipedia.org/wiki/Principle_of_least_astonishmen...

jandrese 4y ago
It's notable that in practically every other programming language on every platform the default is to truncate the file to 0 bytes when opened in write (not append) mode. This isn't one of those things where there are two schools of thought where people take sides. It's a completely baffling decision to buck all convention vs. common sense.
- Someone 4y ago
  > It's notable that in practically every other programming language on every platform the default is to truncate the file to 0 bytes when opened in write (not append) mode Examples of ‘Open’ calls that require setting a bit in the ‘flags’ argument to truncate an existing file: - https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-openfile https://learn.microsoft.com/en-us/windows/win32/api/winbase/... - https://man.freebsd.org/cgi/man.cgi?query=open&sektion=2&n=1 https://man.freebsd.org/cgi/man.cgi?query=open&sektion=2&n=1
  - okanat 4y ago
    You're comparing system call interfaces to higher level standard API defaults. Not really an apples to apples comparison. System calls are special low-level interfaces. The requirement for reading their docs carefully and general care is expected at that level. A higher level API shouldn't contain gotchas like this.
  - moefh 4y ago
    Wow, that Win32 OpenFile function is truly cursed: - If you don't specify a directory, Windows will look for the file in a bunch of directories, but the current directory is not the first place searched. - The lpReOpenBuff is annotated as [out], but is an input when uStyle contains the OF_REOPEN flag. - The function is named OpenFile, but can be used to delete a file. Even better, the documentation is not clear what to do with the return value on success: is it a valid file handle that must be passed to CloseHandle, or just a dummy non-error file handle? - You can also use it to check the existence of the file -- the documentation says it will open and then close the file. What's the return value in that case? Is it a stale file handle, a dummy, or do you still have to close it?
    - Someone 4y ago
      You’re forgetting this one: you can specify a flag that makes the call display a dialog box if a requested file does not exist. Yes, this call to open a file can run an event loop that lets the user create directories, rename files, possibly even format floppy disks, etc. If you want a prime example of a layer violation, this is it. Its replacement “CreateFile” (https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea https://learn.microsoft.com/en-us/windows/win32/api/fileapi/...) is a lot saner, but still has this: “If you rename or delete a file and then restore it shortly afterward, the system searches the cache for file information to restore. Cached information includes its short/long name pair and creation time.” That’s intended to make it easier for “Save” operations in document-based applications to keep the short name of the file, creation time and file flags around.
deleted 4y ago
[deleted]
intelVISA 4y ago
Some say certain agencies wanted it this way.
- jmull 4y ago
  Some are yet naive. “Never attribute to malice that which is adequately explained by stupidity.”
  - account42 4y ago
    Are you trying to volunteer yourself as an example of naivety.
- bagels 4y ago
  Who says this?
  - dylan604 4y ago
    They and them. Don't confuse the pronouns as being more than one person.
- rozab 4y ago
  This would be the stupidest possible way of conducting mass surveillance

amethyst 4y ago

Unrelated: the nostalagia of iliana's favicon is incredible. So much time spent on old modems watching that animation spin while waiting for pages to slowly load in...

neilv 4y ago
That one was great, and stylish. The original NCSA Mosaic one with the globe could've been a bit much, but it was appropriate, given the impact of the Web, even for pre-Web Internet natives: "OMG, this is a tingly step forward in distributed global hypertext, it's happening right now -- interacting around the world, and also superpowering the world." With implied altruistic and benevolent goodness. (The earlier Mcom/Netscape throbbing "N", when I guess they were still rebranding Navigator from Mosaic, looked silly even at the time. Then they did a good one. And there were a bunch of creative alternatives, including some easter egg ones, which JWZ cataloged at one time.)
hu3 4y ago
It's an animated gif! https://iliana.fyi/netscape.gif https://iliana.fyi/netscape.gif

canucker2016 4y ago

Mentioned in the link. This seems to be the original PR for the API in question, ParcelFileDescriptor#parseMode(): https://android.googlesource.com/platform/frameworks/base/+/eb8c3f93edc826413ff4143284dec01c1061d5cc%5E%21/#F3 https://android.googlesource.com/platform/frameworks/base/+/... + public static int parseMode(String mode) { + final int modeBits; + if ("r".equals(mode)) { + modeBits = ParcelFileDescriptor.MODE_READ_ONLY; + } else if ("w".equals(mode) || "wt".equals(mode)) { + modeBits = ParcelFileDescriptor.MODE_WRITE_ONLY + | ParcelFileDescriptor.MODE_CREATE + | ParcelFileDescriptor.MODE_TRUNCATE; + } else if ("wa".equals(mode)) { ... You can see that "w" and "wt" map to the same mode for file operations, including the MODE_TRUNCATE flag. But we jump ahead a few years and the code is reworked. from https://android.googlesource.com/platform/frameworks/base/+/63280e06fc64672ab36d14f852b13df2274cc328%5E%21/core/java/android/os/ParcelFileDescriptor.java https://android.googlesource.com/platform/frameworks/base/+/...: public static int parseMode(String mode) { - final int modeBits; - if ("r".equals(mode)) { - modeBits = ParcelFileDescriptor.MODE_READ_ONLY; - } else if ("w".equals(mode) || "wt".equals(mode)) { - modeBits = ParcelFileDescriptor.MODE_WRITE_ONLY - | ParcelFileDescriptor.MODE_CREATE - | ParcelFileDescriptor.MODE_TRUNCATE; - } else if ("wa".equals(mode)) { ... - return modeBits; + return FileUtils.translateModePosixToPfd(FileUtils.translateModeStringToPosix(mode)); } So what does FileUtils.translateModePosixToPfd(FileUtils.translateModeStringToPosix(mode)) do? from https://android.googlesource.com/platform/frameworks/base/+/63280e06fc64672ab36d14f852b13df2274cc328%5E%21/#F2 https://android.googlesource.com/platform/frameworks/base/+/...: + public static int translateModeStringToPosix(String mode) { + int res = 0; + if (mode.startsWith("rw")) { + res |= O_RDWR | O_CREAT; + } else if (mode.startsWith("w")) { + res |= O_WRONLY | O_CREAT; + } else if (mode.startsWith("r")) { + res |= O_RDONLY; + } else { + throw new IllegalArgumentException("Bad mode: " + mode); + } + if (mode.indexOf('t') != -1) { + res |= O_TRUNC; + } + if (mode.indexOf('a') != -1) { + res |= O_APPEND; + } + return res; + } Now "w" and "wt" map to different Posix open mode flags since one has to explicitly pass in "t" to get file truncation. A dev was careless, plain and simple. Having code in a monorepo doesn't help if you don't check how your code changes affects consumers/clients of the code. If they couldn't change translateModeStringToPosix() behaviour, then the dev could've amended parseMode() to map "w' to "wt" to keep the old behaviour (ugly and non-orthogonal, yes, but that's the way the API was designed).

TazeTSchnitzel 4y ago

I wonder if some developer of an app working with ZIP files will have noticed this bug and had to update their app. ZIP is an unusually terrible file format because it has a footer rather than a header, so the truncation bug would lead to a ZIP file with two headers!

canucker2016 4y ago
Some ZIP-file handling apps go straight to the end of the ZIP file and work backwards looking for the central directory. I believe others will start from the beginning of the file and parse each local directory header and 1) build up their own central directory truth, or 2) stop once they've reached the start of the central directory. I recall that the footer-only format was to enable streaming out a ZIP file esp. when rewinding to the front of the file/stream was not possible, like sending over a network - at least that's one of the use cases I recall.
- ladberg 4y ago
  I believe the footer-only format was more about being able to make modifications by only appending to the zip file and not having to rewrite/shift any data.
  - canucker2016 4y ago
    ...the central directory needs to be shifted by the amount of the new compressed data + local directory header. But that's a lot less than if the central directory was at the beginning and the code had to grow the central directory and shift ALL the compressed data + local directory headers down.
    - ladberg 4y ago
      Yeah that might be the ideal way to do it, but technically you could just append the new entries and new central directory without even removing the old one. I'm not entirely sure what storage system that would make sense on, but presumably there was one back in the day.

neilv 4y ago

Kudos to the engineer who originally not only identified the problem, but also raised the concern about breaking behavior, and about documenting it. Looks like the ball was first dropped when the issue was deferred without acknowledging the reporter's concise suggestions of how to handle it, engineering-wise. Nor a sign that existing code was reviewed for the same problem. Then looks like the ball was dropped again, in what I'm guessing might've been: "old version, forget all the old open issues, all the issues that still apply, someone will rediscover the hard way". This "aCropalypse" event is an example of how making the wrong triage of an issue report can turn out very expensive. All the costs to the world of aCropalypse could've been averted. Some of those costs might eventually come back to the company. It's easy to guess why the problem happened and the report wasn't handled responsibly. By the time the problem exhibited in a way that couldn't be ignored, the pertinent metrics/KPIs/OKRs about clearing issues, resource allocations, and product shipment schedules were already in the past, bonuses had been paid, promotions for shipping new things earned, etc. And security problems are treated as inevitable, even if there's a constant stream of them, and they're produced faster than they're fixed. We're going to need software engineers to be accountable for things we've done and signed off on. (Bonus if accountability changes happen near-term: GPT-powered open source laundering gets a pause, because bridges start falling on everyone foolish enough to sign off on that mangled statistical plagiarism.)

yjftsjthsd-h 4y ago
> We're going to need software engineers to be accountable for things we've done and signed off on. Management first. It's not engineers setting incentives.
- ryandrake 4y ago
  This is just passing the buck. If the boss tells you to write credit card stealing software or to DDOS your competitor are you just going to do it? As professionals, we need to push back and refuse when we know our practices are letting critical bugs go to production, just like (I assume) we'd push back if we were asked to build something unethical. And we should be accountable if we don't. If a civil engineer signs off on a bridge and it collapses during rush hour, he doesn't get to just say, "Whelp, boss rushed me, blame him."

btown 4y ago

A cautionary tale here: when you are testing, if you mock out even low-level I/O utilities, you’re vulnerable to those utilities changing specification subtly. You can supply-chain attack yourself by simply updating a library. Full filesystem/service-engaging integration tests are helpful but far from exhaustive - here, one wouldn’t have just needed to read the cropped file using standard display systems and count the pixels, but hash the underlying file in its entirety. For critical libraries in an I/O pipeline, encourage team members to read CHANGELOGs - both before choosing a library to make sure they’re maintained, and closely when even doing a minor upgrade.

DoctorNick 4y ago
Change logs wouldn’t have helped here, this behavior change was undocumented.
- btown 4y ago
  IMO the Android team should have realized the Android team was not creating internal libraries at a sufficient level of stability and change tracking for the Android team. This quickly gets into cultural priorities that I have very little context on - but I do think that processes failed here.

jimrandomh 4y ago

It looks like this was reported as a normal bug, and the security implications weren't noticed until later. This is unfortunate, but it's also pretty normal and hard to avoid this sort of thing happnening.

layer8 4y ago
It was declared to be an intentional change and not a bug. If anyone would have thought through how an application expecting the old semantics will behave under the new semantics, it should have been rather obvious that silently changing the semantics is a very bad idea. It’s absolutely a case of professional negligence.

sitkack 4y ago

By Design Won't Fix Working As Intended Declaring Bug Bankruptcy Google doesn't test. Google relies on the public to test its software and even then most bugs don't get addressed. If the bug is a gap in the spec or open to interpretation it won't get fixed. Spec bugs are not allowed. The reason that Google has such a high hiring bar is that most of its processes run open-loop and it relies on having smart thorough people to prevent the large swath of issues from being created in the first place.

lapcat 4y ago
> The reason that Google has such a high hiring bar is that most of its processes run open-loop and it relies on having smart thorough people to prevent the large swath of issues from being created in the first place. But there is a large swath of issues. Just look at Google's public bug trackers. And Google's atrocious practice of auto-closing old bug reports partially covers up how many issues there really are.

codethief 4y ago

This reminds me of how a recent Android version (12?) suddenly made certain characters (e.g. ?:<>) illegal in file names[0], probably because to increase compatibility with NTFS. Wtf, Google. Nowhere was this breaking change announced. Not only does my notes app, which has worked flawlessly for years, now crash when I accidentally use one of those characters in a file name / note title. (Unfortunately, the app no longer receives updates.) But suddenly my entire ebook library (which follows the name format "first author & second author: title (year)") no longer synchronizes with my phone (running AOSP / GrapheneOS), either. Fantastic. [0]: https://android.googlesource.com/platform/packages/providers/MediaProvider/+/6b7baf4b9722dd75bfb5d64acadf48223226cde6%5E%21/#F0 https://android.googlesource.com/platform/packages/providers...

dpkirchner 4y ago
Makes you wonder what issue they were mitigating. Passing file names as part of a concatenated string to bash? Ala 'bash -c "ls $filename"'?
- kps 4y ago
  Windows. /** * Sanitizes given name by mutating the file name to make it valid for a FAT filesystem. */ https://cs.android.com/android/platform/superproject/+/master:packages/providers/MediaProvider/src/com/android/providers/media/util/FileUtils.java;drc=master;l=1461 https://cs.android.com/android/platform/superproject/+/maste...
  - codethief 4y ago
    Ah, good point. So it was not NTFS but FAT compatibility they were aiming for.
mikepavone 4y ago
Oh, I was wondering my new phone didn't like some of the music files I was trying to copy over using MTP even though the old one handled them fine. I had assumed the phone manufacturer did something dumb, but apparently it was Google all along.

OnlyRepliesToBS 4y ago

[dead]

cvccvroomvroom 4y ago

Developers at big companies aren't motivated to do everything perfectly. They're graded for big wins, not tech debt, documentation, or correctness.

JoeAltmaier 4y ago

I have sympathy. It's hard to write fundamental APIs. They need to grow and change like anything else, always at the risk of startling somebody. Years ago I extended a network file system server to include additional information in an Open response (revision tag to enable distributed caching). Turns out a customer had also extended the server response in their builds, to do something else security-related. Our release startled them, resulting in a lawsuit. It was all silly and fixed in a patch (a boot config file enable/disable of our new extended info) and it went away. But it underlines how with the best of intentions, still things go wrong.

jasonhansel 4y ago

Why would cropping alter the file in-place? That would presumably leave the image corrupted if (say) your phone died during the cropping process; it would seem much better to save the cropped image separately and then overwrite the original file atomically. After all, it's the same amount of data to write either way.

ledauphin 4y ago
this comment should be higher. atomic move semantics are the single thing that makes writing basic filesystem interactions plausibly deterministic. anything else is just pretending that errors never happen.

lupire 4y ago

> This bug was closed as obsolete a year later. The infamous "Bug Bankruptcy"