4:42 p.m.
At the moment the daemon code contains an incredibly hairy function
called shell_quote for safely quoting strings passed to the shell.
The patch replaces that with a glibc custom printf format (actually
two, but very closely related), %Q and %R.
%Q is like %s but it safely shell quotes the string.
%R is like %Q but it prefixes the path with /sysroot.
Example usage (w/o error checks):
asprintf (&cmd, "zcat %R | tar xvf -", path);
==> "zcat /sysroot/path\ with\ spaces | tar xvf -"
Rich.
[One of the nice things about OCaml is this sort of safe shell quoting
is built in to the stdlib functions].
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
5:09 p.m.
On Mon, Jul 27, 2009 at 10:42:17PM +0100, Richard W.M. Jones wrote:
At the moment the daemon code contains an incredibly hairy function
called shell_quote for safely quoting strings passed to the shell.
The patch replaces that with a glibc custom printf format (actually
two, but very closely related), %Q and %R.
Wow, I didn't know you could register custom printf formats !
+ /* http://udrepper.livejournal.com/20948.html */
+ register_printf_specifier ('Q', print_shell_quote, print_arginfo);
+ register_printf_specifier ('R', print_sysroot_shell_quote, print_arginfo);
Perhaps want to see if you can also make it work with
/* Obsolete interface similar to register_printf_specifier. It can only
handle basic data types because the ARGINFO callback does not return
information on the size of the user-defined type. */
extern int register_printf_function (int __spec, printf_function __func,
printf_arginfo_function __arginfo)
so that libguestfs still compiles on older distros like RHEL-5/EPEL5,
unless they're explicitly out of scope as targets now ?
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5:19 p.m.
New subject: [PATCH] Replace shell_quote function with %Q and %R printf specifiers.
On 27/07/09 22:42, Richard W.M. Jones wrote:
At the moment the daemon code contains an incredibly hairy function
called shell_quote for safely quoting strings passed to the shell.
The patch replaces that with a glibc custom printf format (actually
two, but very closely related), %Q and %R.
%Q is like %s but it safely shell quotes the string.
%R is like %Q but it prefixes the path with /sysroot.
Nice.
There's a danger here, though, of increasing the barrier to
understandability. I can imagine looking at a printf containing %Q or %R
and wasting a huge amount of time on google fruitlessly trying to work
out what on earth it's on about. With that in mind I'd definitely add
something to HACKING. If it weren't for the nowarn wrapper happening to
provide a weirdness hook, I might also put add a comment above every
printf which uses it. I would, however, add a much bigger comment to
asprintf_nowarn, pointing to the implementations.
I also don't think %R is what this feature is about. It seems too
trivial for a fairly off-the-wall trick. You can just as easily write:
#define SYSROOT "/sysroot"
snprintf (cmd, sizeof cmd, "cat " SYSROOT "%s", path);
It's only slightly longer, and a whole lot more readable to the vast
majority of C programmers.
Lastly, avoid using inline. The compiler will inline a function if it
makes sense.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
M: +44 (0)7977 267231
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
3:34 a.m.
On Mon, Jul 27, 2009 at 11:19:00PM +0100, Matthew Booth wrote:
#define SYSROOT "/sysroot"
snprintf (cmd, sizeof cmd, "cat " SYSROOT "%s", path);
This doesn't work because the path must be shell-quoted.
(Also SYSROOT is no longer a fixed string - but now a variable).
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
4:13 a.m.
On 28/07/09 09:34, Richard W.M. Jones wrote:
On Mon, Jul 27, 2009 at 11:19:00PM +0100, Matthew Booth wrote:
> #define SYSROOT "/sysroot"
>
> snprintf (cmd, sizeof cmd, "cat " SYSROOT "%s", path);
This doesn't work because the path must be shell-quoted.
(Also SYSROOT is no longer a fixed string - but now a variable).
Rich.
How about:
snprintf (cmd, sizeof cmd, "cat " SYSROOT "%Q", path);
or:
snprintf (cmd, sizeof cmd, "cat %s%Q", sysroot, path);
All you have to do is ensure that SYSROOT, or sysroot if it's a
variable, is appropriately escaped in advance. This shouldn't be
difficult as it's specified in the program itself.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
M: +44 (0)7977 267231
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
5598
days inactive
5599
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Matthew Booth -
Richard W.M. Jones