[HN Gopher] A very subtle bug (2010)
___________________________________________________________________
A very subtle bug (2010)
Author : r4um
Score : 81 points
Date : 2023-11-20 06:30 UTC (1 days ago)
(HTM) web link (blog.nelhage.com)
(TXT) w3m dump (blog.nelhage.com)
| thirdplace_ wrote:
| i think more bugs reside here.
|
| if tarball or path is user input then they could be used to
| inject tar command options.
| Smaug123 wrote:
| 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.
| ndsipa_pomu wrote:
| 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).
| karatinversion wrote:
| tarball is the mandatory parameter to the preceding -f and
| can't be used to inject commands
| tromp wrote:
| Related: https://news.ycombinator.com/item?id=22647539
| cabirum wrote:
| > 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 wrote:
| 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 wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| xorcist wrote:
| I remain unconvinced that the decision to ignore SIGPIPE
| is right. The description is very hard wavy about it,
| "Python knows what it's doing". Maybe, but I can think of
| half a dozen other situations where this may trip up
| someone who does not take this into consideration.
|
| How do similar languages like Perl do it? They seem to
| work fine doing things the Unix way.
| gumby wrote:
| Pretty clearly a problem with the implementation of Python, not
| a problem with gzip (which is following the POSIX spec) and
| Linux pipes (which are also POSIX compliant).
| josephcsible wrote:
| I'd call it a Python design problem. Python should have never
| let its changes to signal dispositions leak into subprocesses
| by default.
| dveeden2 wrote:
| Maybe better to use something like
| 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...
| mprovost wrote:
| 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 wrote:
| That's silly, there are ways to mark that network operations
| shouldn't cause signals.
| gumby wrote:
| > 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 wrote:
| 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.
| gumby wrote:
| I consider it malpractice
| mjw1007 wrote:
| The attribute for controlling this is implemented but not yet
| stable: https://doc.rust-lang.org/beta/unstable-book/language-
| featur...
| mprovost wrote:
| I'll have to rewrite some chapters once it's stable and
| released but I'm not holding my breath... It will be a nice
| problem to have anyway.
| ramses0 wrote:
| Thank you thank you thank you! Docker processes randomly
| exiting with "137" suddenly make a lot more sense!
|
| 137 - 128 == 9
|
| ... that's SIGKILL, probably via some sort of pipe/subshell.
| We'd figured it was OOM-killer or something similar, but now it
| seems like there's a bit more of the dots connected between.
|
| Who can/will send random `kill -9`s? Probably the kernel, or
| some sort of supervisory process.
| formerly_proven wrote:
| 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 wrote:
| this is clearly fixed by popen starting a docker container for
| the subprocess to run in
| Brian_K_White wrote:
| beershootingoutofnose.gif
| o11c wrote:
| 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 wrote:
| 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/
___________________________________________________________________
(page generated 2023-11-21 23:04 UTC)