Ticket #1037 (closed defect: fixed)
getcluster(1) should quote its shell output
Reported by: | jdreed | Owned by: | |
---|---|---|---|
Priority: | normal | Milestone: | Current Semester |
Component: | -- | Keywords: | |
Cc: | Fixed in version: | ||
Upstream bug: |
Description
See #1036. While OTHER_REPOS=foo,bar ; export OTHER_REPOS ; works, we really should consider quoting.
Change History
comment:3 Changed 13 years ago by geofft
- Owner set to jdreed
- Status changed from development to assigned
It turns out this breaks printing, because the default dotfiles run awk against the output instead of sourcing it, and so you end up with PRINTER set to e.g. "meadow" instead of meadow.
I don't really object to adding quoting, but you need to make sure to update everything that uses it (and decide that nothing out-of-tree uses it without sourcing it).
comment:4 Changed 13 years ago by jdreed
- Status changed from assigned to closed
- Resolution set to wontfix
comment:5 Changed 12 years ago by jdreed
- Status changed from closed to reopened
- Resolution wontfix deleted
- Milestone changed from The Distant Future to Precise Release
comment:6 Changed 12 years ago by jdreed
We probably do want this for a variety of reasons. I'll fix the dotfiles. Any cruft want to chime in about why the dotfiles use awk to begin with instead of sourcing? (For bonus points, please tell me it's to avoid any exploits from getcluster not quoting its shell output)
comment:7 Changed 12 years ago by andersk
We could try to have getcluster quote its output only if it requires quoting, so as not to break backwards compatibility with stupid parsers.
comment:8 Changed 12 years ago by andersk
And/or we could add an better interface (so we don’t have to think about filtering out dangerous variables like PATH). I propose this one:
$ getcluster -h w20-575-1 12.04 APT_RELEASE LPR proposed mitprint $ { read APT_RELEASE; read LPR; } <<EOF $(getcluster -h w20-575-1 12.04 APT_RELEASE LPR) EOF $ echo "APT_RELEASE=$APT_RELEASE LPR=$LPR" APT_RELEASE=foo LPR=bar
comment:9 Changed 12 years ago by jdreed
- Status changed from reopened to development
r25648 (10.1.0-0debathena1) adds a new option to print out space-separated key/value pairs to parse by hand, which is a quick fix for now. In the long term, I like the idea of quoting if the value contains anything other than alphanumeric characters and some whitelisted punctuation, but I also really like the idea of requesting a single value via an option. (getcluster -k APT_RELEASE -h w20-575-1 12.04). I'm going to use this ticket to track the status of the -p fix, and then re-milestone it until we flip out and rewrite getcluster in Python or something.
comment:11 Changed 12 years ago by jdreed
- Fixed in version set to debathena-getcluster 10.1.0-0debathena1
comment:12 Changed 12 years ago by jdreed
- Status changed from proposed to new
- Owner jdreed deleted
- Fixed in version debathena-getcluster 10.1.0-0debathena1 deleted
OK, the short-term fix is in production. Someone should still flip out and do correct shell quoting.
comment:13 Changed 12 years ago by jdreed
- Status changed from new to committed
comment:14 Changed 12 years ago by jdreed
As Anders noted on zephyr, r25865 doesn't actually do much. We instead need to wrap it in single quotes, and replace single quotes in the value with '\'', and also strip (or throw an error) for newlines in tcsh. However, shellenv() also has a bunch of static buffers which should probably go away. At this point, I favor rewriting it in Python or something. Anyone disagree?
comment:15 Changed 12 years ago by jdreed
Python rewrite with quoting functionality committed in r25869. We should not build this until debathena-dotfiles 10.0.32-0debathena1 is also built.
comment:16 Changed 12 years ago by jdreed
dotfiles re-fixed in r25954.
comment:18 Changed 10 years ago by jdreed
- Status changed from development to closed
- Resolution set to fixed
r25378