Richard W.M. Jones wrote:

Subject: [PATCH 2/8] generator: Optional arguments, add-drive-opts (RHBZ#642934,CVE-2010-3851). ...

Hi Rich, I skimmed v1 of this patch and saw nothing wrong. As mentioned on IRC, I did wonder if it'd make sense to warn whenever format=... is omitted. One comment below.

int -guestfs__add_drive_ro_with_if (guestfs_h *g, const char *filename, - const char *drive_if) +guestfs__add_drive_opts (guestfs_h *g, const char *filename, + const struct guestfs_add_drive_opts_argv *optargs) { if (strchr (filename, ',') != NULL) { error (g, _("filename cannot contain ',' (comma) character")); return -1; }

- if (access (filename, F_OK) == -1) { - perrorf (g, "%s", filename); + int readonly = + optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK + ? optargs->readonly : 0; + + const char *iface = + optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK + ? optargs->iface : DRIVE_IF; + + const char *format = + optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_FORMAT_BITMASK + ? optargs->format : NULL; + + /* For writable files, see if we can use cache=off. This also + * checks for the existence of the file. For readonly we have + * to do the check explicitly. + */ + int use_cache_off = readonly ? 0 : test_cache_off (g, filename); + if (use_cache_off == -1) return -1; + + if (readonly) { + if (access (filename, F_OK) == -1) { + perrorf (g, "%s", filename); + return -1; + } }

- size_t len = strlen (filename) + 64; + /* Construct the final -drive parameter. */ + size_t len = 64 + strlen (filename) + strlen (iface); + if (format) len += strlen (format); char buf[len];

- snprintf (buf, len, "file=%s,snapshot=on,if=%s", filename, drive_if); + snprintf (buf, len, "file=%s%s%s%s%s,if=%s", + filename, + readonly ? ",snapshot=on" : "", + use_cache_off ? ",cache=off" : "", + format ? ",format=" : "", + format ? format : "", + iface);

Seeing this makes me wonder if using asprintf would be worthwhile. Of course, that would mean handling allocation failure and freeing the resulting buffer before returning, but it does render the "64" unnecessary, and makes the code a little more robust to addition of a few long_named_option=on options that would exceed the slack provided by 64.

return guestfs__config (g, "-drive", buf); }