Ticket #1037 (closed defect: fixed)

Opened 13 years ago

Last modified 10 years ago

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:1 Changed 13 years ago by jdreed

  • Status changed from new to committed

comment:2 Changed 13 years ago by jdreed

  • Status changed from committed to development

TTL expired

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:10 Changed 12 years ago by jdreed

  • Status changed from development to proposed

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

r25864 fixes the dotfiles.
r25865 fixes getcluster

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:17 Changed 10 years ago by jdreed

  • Status changed from committed to development

comment:18 Changed 10 years ago by jdreed

  • Status changed from development to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.