5 ms·

A very subtle bug (2010)

118 points by r4um 3y ago

thirdplace_ 3y ago
i think more bugs reside here. if tarball or path is user input then they could be used to inject tar command options.
- Smaug123 3y ago
  That, however, is an extremely well-known class of bug. The post is about an extremely obscure (but presumably quite common at the time!) class of bug.
  - carlmr 3y ago
    Also, if you're writing about bug x, you might make a minimal reconstruction of that piece of code only to showcase bug x, instead of the full version with all bells and whistles.
- karatinversion 3y ago
  tarball is the mandatory parameter to the preceding -f and can’t be used to inject commands
- ndsipa_pomu 3y ago
  That's usually prevented by using double dashes ("--") to indicate the end of options, before the file argument. I try to remember to include include double dashes in my shell scripts for as many commands as possible (it's easier to just include them rather than figure out whether a user could possible influence the filename).
  - deleted 3y ago
    [deleted]
tromp 3y ago
Related: https://news.ycombinator.com/item?id=22647539 https://news.ycombinator.com/item?id=22647539
cabirum 3y ago
> This code has a bug From what I see, the code should be expected to work without requiring workarounds; instead, it is one of following: - a python design problem - gzip signal handling bug - a flaw in linux pipe spec
- yccs27 3y ago
  Yeah, the problem is not on the python script side. It seems like a problem with tar or gzip. Tar expects gzip to react graciously to SIGPIPE. Gzip only registers a SIGPIPE handler if SIGPIPE is not ignored. This is either a bug in gzip, or tar has to make sure it starts gzip without SIGPIPE ignored. This doesn't mean the failure is any less complex. Tar incorrectly assumes that starting gzip without extra setup doesn't make it ignore SIGPIPE, which is subtly wrong.
  - ndsipa_pomu 3y ago
    > the problem is not on the python script side. It's a problem with tar or gzip As it works perfectly in the equivalent shell script, I'd point the finger at Python for not wanting to handle SIGPIPE signals.
    - yccs27 3y ago
      Is tar really only expected to work in a standard shell script setup? It's a general-purpose utility after all. Python not wanting to handle SIGPIPE is perfectly fine. It is less clear whether it's fine for python to keep it disabled for subprocesses. The other side: Is it okay for `tar` to fail if it is started with SIGPIPE disabled? That's definitely not what I'd expect, although it's maybe excusable since disabled SIGPIPE is nonstandard. You can argue about whether a system utility like tar is expected to handle nonstandard conditions -- I'd tend towards 'yes'.
      - torstenvl 3y ago
        If you're going to use popen(), you have to be prepared to handle how the utility you call sends data over that pipe. The bug is indisputably on the Python script side unless there's a spec tar claims to adhere to but doesn't. That said, I don't think it's an unreasonable bug. I certainly wouldn't think any less of a programmer who wrote it.
dveeden2 3y ago
Maybe better to use something like https://docs.python.org/3/library/tarfile.html https://docs.python.org/3/library/tarfile.html instead of calling some "outside" utilities? This might be safer (command injection) and maybe more reliable. However I don't know what I would have chosen and hindsight is always 20/20 and there might be other external commands and requirements...
- kmill 3y ago
  If you're wondering about downvotes, it's probably because this is mentioned briefly. > It probably should be using tarfile, but let’s ignore that for the moment.
  - carlmr 3y ago
    Apart from that it's mentioned. I have often seen tarfile being replaced by tar as subprocess, because tarfile is just so slow that it can make or break your use-case. I'm not sure if tarfile got better since I've last used it.
mprovost 3y ago
I've run into a similar issue in Rust with SIGPIPE. The compiler adds the call to signal() to ignore SIGPIPEs when a Rust program starts. Apparently this was to stop a listening server from exiting when a client closed their connection. But in my book I'm teaching Rust by rewriting a classic BSD utility (cat, head, wc, ...) in Rust and none of those utilities have to care about broken pipes because they don't ignore the signal and the OS silently kills the process. Instead, if a Rust program doesn't handle a write failing with a EPIPE error then the process panics and exits with a backtrace. The worst part is that Rust standard library doesn't include a module for handling signals so it's not possible to undo the signal() call ignoring SIGPIPEs without using another crate. In the end it's fine and you just have to handle broken pipes but it adds a lot of boilerplate to small CLI programs that in C just work as expected. Another twist (and possible further subtle bug) is that most shells set the exit status of a program that exits from a broken pipe to 141 (128 + the signal number, in this case 13). So when you catch the pipe you can exit the process with a status of 141, but that's the shell's behaviour and there's no safe way to fake an exit status.
- mgaunard 3y ago
  That's silly, there are ways to mark that network operations shouldn't cause signals.
- gumby 3y ago
  > The compiler adds the call to signal() to ignore SIGPIPEs when a Rust program starts. Apparently this was to stop a listening server from exiting when a client closed their connection That’s appalling and irresponsible if true. Not every program is a server! If you want to ignore SIGPIPE you can do that yourself.
  - mprovost 3y ago
    Well, that's the source of the bug in Python as well so it's not exactly a unique choice or default. It's tricky to manage this in a cross-platform way.
    - 3y ago
formerly_proven 3y ago
In my experience virtually every use of the subprocess module at the time of first writing contains at least one more or less subtle bug.
tbm57 3y ago
this is clearly fixed by popen starting a docker container for the subprocess to run in
- Brian_K_White 3y ago
  beershootingoutofnose.gif
  - actionfromafar 3y ago
    The inevitable error messages can be explained by an LLM?
  - shoo 3y ago
    periodic nasal irrigation with beer also helps mitigate the impact of nasal demons. although, they may briefly become unusually rowdy before they pass out.
o11c 3y ago
The "fix" also has a bug - `preexec_fn` causes nasal demons if the current process has threads. Fortunately, Python 3.2 added the `restore_signals` argument for exactly this purpose, and it is enabled by default. The (2010) in the title is quite relevant.
- interroboink 3y ago
  Though from the bug report, the fix did not make it into Python 2.7 ... still being used some places today — by other people, not me (: There is a backport fixed version though: https://code.google.com/archive/p/python-subprocess32/ https://code.google.com/archive/p/python-subprocess32/

118 points by r4um 3y ago

thirdplace_ 3y ago

i think more bugs reside here. if tarball or path is user input then they could be used to inject tar command options.

Smaug123 3y ago
That, however, is an extremely well-known class of bug. The post is about an extremely obscure (but presumably quite common at the time!) class of bug.
- carlmr 3y ago
  Also, if you're writing about bug x, you might make a minimal reconstruction of that piece of code only to showcase bug x, instead of the full version with all bells and whistles.
karatinversion 3y ago
tarball is the mandatory parameter to the preceding -f and can’t be used to inject commands
ndsipa_pomu 3y ago
That's usually prevented by using double dashes ("--") to indicate the end of options, before the file argument. I try to remember to include include double dashes in my shell scripts for as many commands as possible (it's easier to just include them rather than figure out whether a user could possible influence the filename).
- deleted 3y ago
  [deleted]

tromp 3y ago

Related: https://news.ycombinator.com/item?id=22647539 https://news.ycombinator.com/item?id=22647539

cabirum 3y ago

> This code has a bug From what I see, the code should be expected to work without requiring workarounds; instead, it is one of following: - a python design problem - gzip signal handling bug - a flaw in linux pipe spec

yccs27 3y ago
Yeah, the problem is not on the python script side. It seems like a problem with tar or gzip. Tar expects gzip to react graciously to SIGPIPE. Gzip only registers a SIGPIPE handler if SIGPIPE is not ignored. This is either a bug in gzip, or tar has to make sure it starts gzip without SIGPIPE ignored. This doesn't mean the failure is any less complex. Tar incorrectly assumes that starting gzip without extra setup doesn't make it ignore SIGPIPE, which is subtly wrong.
- ndsipa_pomu 3y ago
  > the problem is not on the python script side. It's a problem with tar or gzip As it works perfectly in the equivalent shell script, I'd point the finger at Python for not wanting to handle SIGPIPE signals.
  - yccs27 3y ago
    Is tar really only expected to work in a standard shell script setup? It's a general-purpose utility after all. Python not wanting to handle SIGPIPE is perfectly fine. It is less clear whether it's fine for python to keep it disabled for subprocesses. The other side: Is it okay for `tar` to fail if it is started with SIGPIPE disabled? That's definitely not what I'd expect, although it's maybe excusable since disabled SIGPIPE is nonstandard. You can argue about whether a system utility like tar is expected to handle nonstandard conditions -- I'd tend towards 'yes'.
    - torstenvl 3y ago
      If you're going to use popen(), you have to be prepared to handle how the utility you call sends data over that pipe. The bug is indisputably on the Python script side unless there's a spec tar claims to adhere to but doesn't. That said, I don't think it's an unreasonable bug. I certainly wouldn't think any less of a programmer who wrote it.

dveeden2 3y ago

Maybe better to use something like https://docs.python.org/3/library/tarfile.html https://docs.python.org/3/library/tarfile.html instead of calling some "outside" utilities? This might be safer (command injection) and maybe more reliable. However I don't know what I would have chosen and hindsight is always 20/20 and there might be other external commands and requirements...

kmill 3y ago
If you're wondering about downvotes, it's probably because this is mentioned briefly. > It probably should be using tarfile, but let’s ignore that for the moment.
- carlmr 3y ago
  Apart from that it's mentioned. I have often seen tarfile being replaced by tar as subprocess, because tarfile is just so slow that it can make or break your use-case. I'm not sure if tarfile got better since I've last used it.

mprovost 3y ago

I've run into a similar issue in Rust with SIGPIPE. The compiler adds the call to signal() to ignore SIGPIPEs when a Rust program starts. Apparently this was to stop a listening server from exiting when a client closed their connection. But in my book I'm teaching Rust by rewriting a classic BSD utility (cat, head, wc, ...) in Rust and none of those utilities have to care about broken pipes because they don't ignore the signal and the OS silently kills the process. Instead, if a Rust program doesn't handle a write failing with a EPIPE error then the process panics and exits with a backtrace. The worst part is that Rust standard library doesn't include a module for handling signals so it's not possible to undo the signal() call ignoring SIGPIPEs without using another crate. In the end it's fine and you just have to handle broken pipes but it adds a lot of boilerplate to small CLI programs that in C just work as expected. Another twist (and possible further subtle bug) is that most shells set the exit status of a program that exits from a broken pipe to 141 (128 + the signal number, in this case 13). So when you catch the pipe you can exit the process with a status of 141, but that's the shell's behaviour and there's no safe way to fake an exit status.

mgaunard 3y ago
That's silly, there are ways to mark that network operations shouldn't cause signals.
gumby 3y ago
> The compiler adds the call to signal() to ignore SIGPIPEs when a Rust program starts. Apparently this was to stop a listening server from exiting when a client closed their connection That’s appalling and irresponsible if true. Not every program is a server! If you want to ignore SIGPIPE you can do that yourself.
- mprovost 3y ago
  Well, that's the source of the bug in Python as well so it's not exactly a unique choice or default. It's tricky to manage this in a cross-platform way.
  - 3y ago

formerly_proven 3y ago

In my experience virtually every use of the subprocess module at the time of first writing contains at least one more or less subtle bug.

tbm57 3y ago

this is clearly fixed by popen starting a docker container for the subprocess to run in

Brian_K_White 3y ago
beershootingoutofnose.gif
- actionfromafar 3y ago
  The inevitable error messages can be explained by an LLM?
- shoo 3y ago
  periodic nasal irrigation with beer also helps mitigate the impact of nasal demons. although, they may briefly become unusually rowdy before they pass out.

o11c 3y ago

The "fix" also has a bug - `preexec_fn` causes nasal demons if the current process has threads. Fortunately, Python 3.2 added the `restore_signals` argument for exactly this purpose, and it is enabled by default. The (2010) in the title is quite relevant.

interroboink 3y ago
Though from the bug report, the fix did not make it into Python 2.7 ... still being used some places today — by other people, not me (: There is a backport fixed version though: https://code.google.com/archive/p/python-subprocess32/ https://code.google.com/archive/p/python-subprocess32/