[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



"Richard W.M. Jones" <rjones redhat com> writes:
> 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-FILE-FORMAT-HEADER
>> +   */
>> +
>> +  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.

Ok, I'll revise to use those instead.

>> +  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?

As the patch is presently written, one could create a malicious RPM such
that the value read into num_fields is prohibitively large, but I'd
expect it to segfault once cursor overruns the memory for the header.
This should be bound by header_len, which somewhere along the way in my
refactorings I botched.  Good catch, I'll fix that.

>> +  }
>> +
>> +  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).

Aye.  I originally used just "" but I changed it to match the output
that RPM gives.  I'll put it back.

>>    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?

I tested it against a F17 guest I had, and it correctly pulls the arch
out and displays it.  However, I just realized that it doesn't return
both versions of the package.  For example, if I have glibc.x86_64 and
glibc.i686 installed, only one of them is returned in the output.  I
need to fix that as well.  But the header parsing works.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]