Re: [Libguestfs] [PATCH] lib: update inspect_list_applications to return app_arch (RHBZ#859949)

On Mon, Oct 15, 2012 at 04:30:24PM -0400, John Eckersberg wrote: > (Work in progress) > > For now this only works for RPM, the others just return an empty string. > --- > generator/structs.ml | 1 + > src/inspect-apps.c | 88 +++++++++++++++++++++++++++++----------------------- > 2 files changed, 51 insertions(+), 38 deletions(-) > > diff --git a/generator/structs.ml b/generator/structs.ml > index d62fcc5..8ac9fc5 100644 > --- a/generator/structs.ml > +++ b/generator/structs.ml > @@ -183,6 +183,7 @@ let structs = [ > "app_epoch", FInt32; > "app_version", FString; > "app_release", FString; > + "app_arch", FString; > "app_install_path", FString; > "app_trans_path", FString; > "app_publisher", FString; Unfortunately (and this a rather serious shortcoming of the generator) you cannot add fields to an existing struct and maintain backwards binary compatibility. Let's not worry about this bit right now though, I need to think about what to do a bit more. > diff --git a/src/inspect-apps.c b/src/inspect-apps.c > index f65c70a..3dbfb5b 100644 > --- a/src/inspect-apps.c > +++ b/src/inspect-apps.c > @@ -31,6 +31,7 @@ > #ifdef HAVE_ENDIAN_H > #include <endian.h> > #endif > +#include <arpa/inet.h> Don't need this -- see below. > #include <pcre.h> > > @@ -46,7 +47,7 @@ static struct guestfs_application_list *list_applications_rpm (guestfs_h *g, str > #endif > static struct guestfs_application_list *list_applications_deb (guestfs_h *g, struct inspect_fs *fs); > static struct guestfs_application_list *list_applications_windows (guestfs_h *g, struct inspect_fs *fs); > -static void add_application (guestfs_h *g, struct guestfs_application_list *, const char *name, const char *display_name, int32_t epoch, const char *version, const char *release, const char *install_path, const char *publisher, const char *url, const char *description); > +static void add_application (guestfs_h *g, struct guestfs_application_list *, const char *name, const char *display_name, int32_t epoch, const char *version, const char *release, const char *arch, const char *install_path, const char *publisher, const char *url, const char *description); > static void sort_applications (struct guestfs_application_list *); > > /* Unlike the simple inspect-get-* calls, this one assumes that the > @@ -181,6 +182,37 @@ read_rpm_name (guestfs_h *g, > return 0; > } > > +/* tag constants, see rpmtag.h in RPM for complete list */ > +#define RPMTAG_VERSION 1001 > +#define RPMTAG_RELEASE 1002 > +#define RPMTAG_ARCH 1022 > + > +static char * > +get_rpm_header_tag (guestfs_h *g, const void *header_start, size_t header_len, uint32_t tag) > +{ > + uint32_t num_fields, offset; > + const void *cursor = header_start + 8, *store; > + > + /* This function parses the RPM header structure to pull out various > + * tag strings (version, release, arch, etc.). For more detail on the > + * header format, see: > + * http://www.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html#S2-RPM... > + */ > + > + num_fields = ntohl (*(uint32_t *) header_start); It's probably better to use the functions from <endian.h> like 'be32toh', 'le32toh' etc. We already use them, eg. in src/inspect-fs-windows.c.

> + store = header_start + 8 + (16 * num_fields); > + > + while (cursor < store ){ > + if (ntohl (*(uint32_t *) cursor) == tag){ > + offset = ntohl(*(uint32_t *) (cursor + 8)); > + return safe_strdup(g, store + offset); > + } > + cursor += 16; Can a malicious RPM in the guest cause an infinite loop in the host?

> + } > + > + return NULL; > +} > + > struct read_package_data { > struct rpm_names_list *list; > struct guestfs_application_list *apps; > @@ -194,16 +226,13 @@ read_package (guestfs_h *g, > { > struct read_package_data *data = datav; > struct rpm_name nkey, *entry; > - char *p; > - size_t len; > - ssize_t max; > - char *nul_name_nul, *version, *release; > + char *version, *release, *arch; > > /* This function reads one (key, value) pair from the Packages > * database. The key is the link field (see struct rpm_name). The > - * value is a long binary string, but we can extract the version > - * number from it as below. First we have to look up the link field > - * in the list of links (which is sorted by link field). > + * value is a long binary string, but we can extract the header data > + * from it as below. First we have to look up the link field in the > + * list of links (which is sorted by link field). > */ > > /* Ignore bogus entries. */ > @@ -219,41 +248,23 @@ read_package (guestfs_h *g, > > /* We found a matching link entry, so that gives us the application > * name (entry->name). Now we can get other data for this > - * application out of the binary value string. XXX This is a real > - * hack. > + * application out of the binary value string. > */ > > - /* Look for \0<name>\0 */ > - len = strlen (entry->name); > - nul_name_nul = safe_malloc (g, len + 2); > - nul_name_nul[0] = '\0'; > - memcpy (&nul_name_nul[1], entry->name, len); > - nul_name_nul[len+1] = '\0'; > - p = memmem (value, valuelen, nul_name_nul, len+2); > - free (nul_name_nul); > - if (!p) > - return 0; > - > - /* Following that are \0-delimited version and release fields. */ > - p += len + 2; /* Note we have to skip \0 + name + \0. */ > - max = valuelen - (p - (char *) value); > - if (max < 0) > - max = 0; > - version = safe_strndup (g, p, max); > - > - len = strlen (version); > - p += len + 1; > - max = valuelen - (p - (char *) value); > - if (max < 0) > - max = 0; > - release = safe_strndup (g, p, max); > + version = get_rpm_header_tag(g, value, valuelen, RPMTAG_VERSION); > + release = get_rpm_header_tag(g, value, valuelen, RPMTAG_RELEASE); > + arch = get_rpm_header_tag(g, value, valuelen, RPMTAG_ARCH); Good, this seems like a considerable improvement. > /* Add the application and what we know. */ > add_application (g, data->apps, entry->name, "", 0, version, release, > - "", "", "", ""); > + arch ? arch : "(none)", "", "", "", ""); Don't use "(none)". If you need a special value, use either "" or "unknown" (I suspect in this case empty string), and document it in the API (generator/actions.ml).

> free (version); > free (release); > + if (arch) > + /* arch will occasionally be NULL, because some packages > + * (e.g. "gpgkeys") do not have an arch */ > + free (arch); > > return 0; > } > @@ -378,7 +389,7 @@ list_applications_deb (guestfs_h *g, struct inspect_fs *fs) > else if (STREQ (line, "")) { > if (installed_flag && name && version) > add_application (g, apps, name, "", 0, version, release ? : "", > - "", "", "", ""); > + "", "", "", "", ""); > free (name); > free (version); > free (release); > @@ -519,7 +530,7 @@ list_applications_windows_from_path (guestfs_h *g, > > add_application (g, apps, name, display_name, 0, > version ? : "", > - "", > + "", "", > install_path ? : "", > publisher ? : "", > url ? : "", > @@ -542,7 +553,7 @@ list_applications_windows_from_path (guestfs_h *g, > static void > add_application (guestfs_h *g, struct guestfs_application_list *apps, > const char *name, const char *display_name, int32_t epoch, > - const char *version, const char *release, > + const char *version, const char *release, const char *arch, > const char *install_path, > const char *publisher, const char *url, > const char *description) > @@ -555,6 +566,7 @@ add_application (guestfs_h *g, struct guestfs_application_list *apps, > apps->val[apps->len-1].app_epoch = epoch; > apps->val[apps->len-1].app_version = safe_strdup (g, version); > apps->val[apps->len-1].app_release = safe_strdup (g, release); > + apps->val[apps->len-1].app_arch = safe_strdup (g, arch); > apps->val[apps->len-1].app_install_path = safe_strdup (g, install_path); > /* XXX Translated path is not implemented yet. */ > apps->val[apps->len-1].app_trans_path = safe_strdup (g, ""); > -- > 1.7.11.4 Does it work?