GNOME.org
 

[gcab] Use g_autoptr() to fix countless memory leaks when parsing corrupt files

commit b4e44c181619e58f16881aa6fbf9d906872bed85
Author: Richard Hughes <richard hughsie com>
Date:   Wed Dec 13 11:59:52 2017 +0000

    Use g_autoptr() to fix countless memory leaks when parsing corrupt files
    
    Also, fix a memory leak when loading archives with authenticode signatures.

 libgcab/cabinet.c      |   31 ++++++++++++++++++++
 libgcab/cabinet.h      |   31 ++++++++++++--------
 libgcab/gcab-cabinet.c |   73 ++++++++++++++++++++++++++++-------------------
 libgcab/gcab-file.c    |   60 +++++++++++++++++++++------------------
 libgcab/gcab-folder.c  |   19 ++++++++----
 libgcab/gcab-priv.h    |    4 +-
 6 files changed, 139 insertions(+), 79 deletions(-)
---
diff --git a/libgcab/cabinet.c b/libgcab/cabinet.c
index f7b8af3..221a6eb 100644
--- a/libgcab/cabinet.c
+++ b/libgcab/cabinet.c
@@ -311,6 +311,19 @@ end:
     return success;
 }
 
+void
+cheader_free (cheader_t *ch)
+{
+    if (ch == NULL)
+        return;
+    g_free (ch->reserved);
+    g_free (ch->cab_prev);
+    g_free (ch->disk_prev);
+    g_free (ch->cab_next);
+    g_free (ch->disk_next);
+    g_free (ch);
+}
+
 G_GNUC_INTERNAL gboolean
 cfolder_write (cfolder_t *cf, GDataOutputStream *out,
                GCancellable *cancellable, GError **error)
@@ -350,6 +363,15 @@ end:
     return success;
 }
 
+void
+cfolder_free (cfolder_t *cf)
+{
+    if (cf == NULL)
+        return;
+    g_free (cf->reserved);
+    g_free (cf);
+}
+
 G_GNUC_INTERNAL gboolean
 cfile_write (cfile_t *cf, GDataOutputStream *out,
              GCancellable *cancellable, GError **error)
@@ -398,6 +420,15 @@ end:
     return success;
 }
 
+void
+cfile_free (cfile_t *cf)
+{
+    if (cf == NULL)
+        return;
+    g_free (cf->name);
+    g_free (cf);
+}
+
 static guint32
 compute_checksum (guint8 *in, guint16 ncbytes, guint32 seed)
 {
diff --git a/libgcab/cabinet.h b/libgcab/cabinet.h
index c0db5ce..0a2b873 100644
--- a/libgcab/cabinet.h
+++ b/libgcab/cabinet.h
@@ -40,17 +40,12 @@
 /* based on the spec
    http://msdn.microsoft.com/en-us/library/bb417343.aspx */
 
-typedef struct cheader cheader_t;
-typedef struct cfolder cfolder_t;
-typedef struct cfile cfile_t;
-typedef struct cdata cdata_t;
-
 #define DATABLOCKSIZE           32768
 
 #define CFO_START               0x24    /* folder offset */
 #define CFI_START               0x2C    /* file offset */
 
-struct cheader
+typedef struct
 {
     guint32 res1;
     guint32 size;
@@ -72,7 +67,7 @@ struct cheader
     gchar *disk_prev;
     gchar *cab_next;
     gchar *disk_next;
-};
+} cheader_t;
 
 typedef enum {
     CABINET_HEADER_PREV = 0x0001,
@@ -80,15 +75,15 @@ typedef enum {
     CABINET_HEADER_RESERVE = 0x0004,
 } CabinetHeaderFlags;
 
-struct cfolder
+typedef struct
 {
     guint32 offsetdata;
     guint16 ndatab;
     guint16 typecomp;
     guint8 *reserved;
-};
+} cfolder_t;
 
-struct cfile
+typedef struct
 {
     guint32 usize;
     guint32 uoffset;
@@ -97,9 +92,9 @@ struct cfile
     guint16 time;
     guint16 fattr;
     gchar *name;
-};
+} cfile_t;
 
-struct cdata
+typedef struct
 {
     guint32 checksum;
     guint16 ncbytes;
@@ -112,7 +107,7 @@ struct cdata
     /* using wine decomp.h */
     FDI_Int fdi;
     fdi_decomp_state decomp;
-};
+} cdata_t;
 
 gboolean     cheader_write                      (cheader_t *ch,
                                                  GDataOutputStream *out,
@@ -122,6 +117,8 @@ gboolean     cheader_read                       (cheader_t *ch,
                                                  GDataInputStream *in,
                                                  GCancellable *cancellable,
                                                  GError **error);
+void         cheader_free                       (cheader_t *ch);
+
 gboolean     cfolder_write                      (cfolder_t *cf,
                                                  GDataOutputStream *out,
                                                  GCancellable *cancellable,
@@ -131,6 +128,8 @@ gboolean     cfolder_read                       (cfolder_t *cf,
                                                  GDataInputStream *in,
                                                  GCancellable *cancellable,
                                                  GError **error);
+void         cfolder_free                       (cfolder_t *cf);
+
 gboolean     cfile_write                        (cfile_t *cf,
                                                  GDataOutputStream *out,
                                                  GCancellable *cancellable,
@@ -139,6 +138,8 @@ gboolean     cfile_read                         (cfile_t *cf,
                                                  GDataInputStream *in,
                                                  GCancellable *cancellable,
                                                  GError **error);
+void         cfile_free                         (cfile_t *cf);
+
 gboolean     cdata_write                        (cdata_t *cd,
                                                  GDataOutputStream *out,
                                                  int type,
@@ -155,4 +156,8 @@ gboolean     cdata_read                         (cdata_t *cd,
                                                  GError **error);
 void         cdata_finish                       (cdata_t *cd, GError **error);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(cfolder_t, cfolder_free)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(cfile_t, cfile_free)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(cheader_t, cheader_free)
+
 #endif /* CABINET_H */
diff --git a/libgcab/gcab-cabinet.c b/libgcab/gcab-cabinet.c
index f2405ae..ebe486e 100644
--- a/libgcab/gcab-cabinet.c
+++ b/libgcab/gcab-cabinet.c
@@ -41,7 +41,7 @@ struct _GCabCabinet {
 
     GPtrArray *folders;
     GByteArray *reserved;
-    cheader_t cheader;
+    cheader_t *cheader;
     GByteArray *signature;
     GInputStream *stream;
 };
@@ -72,6 +72,7 @@ gcab_cabinet_finalize (GObject *object)
 {
     GCabCabinet *self = GCAB_CABINET (object);
 
+    cheader_free (self->cheader);
     g_ptr_array_unref (self->folders);
     if (self->reserved)
         g_byte_array_unref (self->reserved);
@@ -407,57 +408,65 @@ gcab_cabinet_load (GCabCabinet *self,
     g_return_val_if_fail (self->folders->len == 0, FALSE);
     g_return_val_if_fail (self->stream == NULL, FALSE);
 
-    self->stream = g_object_ref (stream);
-
-    cheader_t cheader;
+    g_autoptr(cheader_t) cheader = NULL;
     g_autoptr(GDataInputStream) in = g_data_input_stream_new (stream);
     g_filter_input_stream_set_close_base_stream (G_FILTER_INPUT_STREAM (in), FALSE);
     g_data_input_stream_set_byte_order (in, G_DATA_STREAM_BYTE_ORDER_LITTLE_ENDIAN);
 
-    if (!cheader_read (&cheader, in, cancellable, error))
+    cheader = g_new0 (cheader_t, 1);
+    if (!cheader_read (cheader, in, cancellable, error))
         return FALSE;
 
-    if (cheader.reserved)
-        g_object_set (self, "reserved",
-                      g_byte_array_new_take (cheader.reserved, cheader.res_header),
-                      NULL);
+    if (cheader->reserved != NULL) {
+        g_autoptr(GByteArray) blob = NULL;
+        blob = g_byte_array_new_take (cheader->reserved, cheader->res_header);
+        g_object_set (self, "reserved", blob, NULL);
+        cheader->reserved = NULL;
+    }
 
-    for (guint i = 0; i < cheader.nfolders; i++) {
-        cfolder_t cfolder = { 0, };
-        if (!cfolder_read (&cfolder, cheader.res_folder, in, cancellable, error))
+    for (guint i = 0; i < cheader->nfolders; i++) {
+        g_autoptr(cfolder_t) cfolder = g_new0 (cfolder_t, 1);
+        g_autoptr(GByteArray) blob = NULL;
+        if (!cfolder_read (cfolder, cheader->res_folder, in, cancellable, error))
             return FALSE;
 
-        GCabFolder *folder = gcab_folder_new_with_cfolder (&cfolder, stream);
-        if (cfolder.reserved)
-            g_object_set (folder, "reserved",
-                          g_byte_array_new_take (cfolder.reserved, cheader.res_folder),
-                          NULL);
+        /* steal this inelegantly */
+        if (cfolder->reserved != NULL) {
+            blob = g_byte_array_new_take (cfolder->reserved, cheader->res_folder);
+            cfolder->reserved = NULL;
+        }
+
+        GCabFolder *folder = gcab_folder_new_steal_cfolder (&cfolder, stream);
+        if (blob != NULL)
+            g_object_set (folder, "reserved", blob, NULL);
         g_ptr_array_add (self->folders, folder);
-        cfolder.reserved = NULL;
     }
 
-    for (guint i = 0; i < cheader.nfiles; i++) {
-        cfile_t cfile = { 0, };
-        if (!cfile_read (&cfile, in, cancellable, error))
+    for (guint i = 0; i < cheader->nfiles; i++) {
+        g_autoptr(cfile_t) cfile = g_new0 (cfile_t, 1);
+        if (!cfile_read (cfile, in, cancellable, error))
             return FALSE;
 
-        if (cfile.index >= self->folders->len) {
+        if (cfile->index >= self->folders->len) {
             g_set_error (error, GCAB_ERROR, GCAB_ERROR_FORMAT,
                          "Invalid folder index");
             return FALSE;
         }
 
-        GCabFolder *folder = g_ptr_array_index (self->folders, cfile.index);
+        GCabFolder *folder = g_ptr_array_index (self->folders, cfile->index);
         if (folder == NULL) {
             g_set_error (error, GCAB_ERROR, GCAB_ERROR_FORMAT,
                          "Invalid folder pointer");
             return FALSE;
         }
 
-        g_autoptr(GCabFile) file = gcab_file_new_with_cfile (&cfile);
+        g_autoptr(GCabFile) file = gcab_file_new_steal_cfile (&cfile);
         if (!gcab_folder_add_file (folder, file, FALSE, cancellable, error))
             return FALSE;
     }
+
+    self->stream = g_object_ref (stream);
+    self->cheader = g_steal_pointer (&cheader);
     return TRUE;
 }
 
@@ -486,24 +495,28 @@ gcab_cabinet_extract (GCabCabinet *self,
                       GCancellable *cancellable,
                       GError **error)
 {
-    gboolean success = TRUE;
-
     g_return_val_if_fail (GCAB_IS_CABINET (self), FALSE);
     g_return_val_if_fail (G_IS_FILE (path), FALSE);
     g_return_val_if_fail (!cancellable || G_IS_CANCELLABLE (cancellable), FALSE);
     g_return_val_if_fail (!error || *error == NULL, FALSE);
 
+    /* never loaded from a stream */
+    if (self->cheader == NULL) {
+        g_set_error (error, GCAB_ERROR, GCAB_ERROR_FAILED,
+                     "Cabinet has not been loaded");
+        return FALSE;
+    }
+
     for (guint i = 0; i < self->folders->len; ++i) {
         GCabFolder *folder = g_ptr_array_index (self->folders, i);
-        if (!gcab_folder_extract (folder, path, self->cheader.res_data,
+        if (!gcab_folder_extract (folder, path, self->cheader->res_data,
                                   file_callback, progress_callback, user_data,
                                   cancellable, error)) {
-            success = FALSE;
-            break;
+            return FALSE;
         }
     }
 
-    return success;
+    return TRUE;
 }
 
 /**
diff --git a/libgcab/gcab-file.c b/libgcab/gcab-file.c
index ec7c615..11e2368 100644
--- a/libgcab/gcab-file.c
+++ b/libgcab/gcab-file.c
@@ -45,7 +45,7 @@ struct _GCabFile
 
     gchar *extract_name;
     GFile *file;
-    cfile_t cfile;
+    cfile_t *cfile;
 };
 
 enum {
@@ -69,7 +69,7 @@ gcab_file_finalize (GObject *object)
 
     if (self->file != NULL)
         g_object_unref (self->file);
-    g_free (self->cfile.name);
+    cfile_free (self->cfile);
     g_free (self->extract_name);
 
     G_OBJECT_CLASS (gcab_file_parent_class)->finalize (object);
@@ -80,6 +80,8 @@ gcab_file_set_name (GCabFile *self, const gchar *name)
 {
     gchar *fname = g_strdup (name);
 
+    g_return_if_fail (self->cfile != NULL);
+
     /* assuming that on win32 we don't get unix paths */
 #ifndef G_OS_WIN32
     if (fname) {
@@ -90,8 +92,8 @@ gcab_file_set_name (GCabFile *self, const gchar *name)
     }
 #endif
 
-    g_free (self->cfile.name);
-    self->cfile.name = fname;
+    g_free (self->cfile->name);
+    self->cfile->name = fname;
 }
 
 static void
@@ -121,7 +123,7 @@ gcab_file_get_property (GObject *object, guint prop_id, GValue *value, GParamSpe
 
     switch (prop_id) {
     case PROP_NAME:
-        g_value_set_string (value, self->cfile.name);
+        g_value_set_string (value, self->cfile->name);
         break;
     case PROP_FILE:
         g_value_set_object (value, self->file);
@@ -143,12 +145,12 @@ gcab_file_class_init (GCabFileClass *klass)
 
     g_object_class_install_property (object_class, PROP_NAME,
         g_param_spec_string ("name", "name", "name", NULL,
-                             G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE |
+                             G_PARAM_READWRITE |
                              G_PARAM_STATIC_STRINGS));
 
     g_object_class_install_property (object_class, PROP_FILE,
         g_param_spec_object ("file", "file", "file", G_TYPE_FILE,
-                             G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE |
+                             G_PARAM_READWRITE |
                              G_PARAM_STATIC_STRINGS));
 }
 
@@ -166,11 +168,11 @@ gcab_file_update_info (GCabFile *self, GFileInfo *info)
     time = tv.tv_sec;
     m = gmtime (&time);
 
-    self->cfile.usize = g_file_info_get_size (info);
-    self->cfile.fattr = GCAB_FILE_ATTRIBUTE_ARCH;
-    self->cfile.date = ((m->tm_year + 1900 - 1980 ) << 9 ) +
+    self->cfile->usize = g_file_info_get_size (info);
+    self->cfile->fattr = GCAB_FILE_ATTRIBUTE_ARCH;
+    self->cfile->date = ((m->tm_year + 1900 - 1980 ) << 9 ) +
         ((m->tm_mon+1) << 5 ) + (m->tm_mday);
-    self->cfile.time = (m->tm_hour << 11) + (m->tm_min << 5) + (m->tm_sec / 2);
+    self->cfile->time = (m->tm_hour << 11) + (m->tm_min << 5) + (m->tm_sec / 2);
 
     return TRUE;
 }
@@ -180,7 +182,7 @@ gcab_file_set_uoffset (GCabFile *self, guint32 uoffset)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), FALSE);
 
-    self->cfile.uoffset = uoffset;
+    self->cfile->uoffset = uoffset;
 
     return TRUE;
 }
@@ -189,14 +191,14 @@ G_GNUC_INTERNAL guint32
 gcab_file_get_uoffset (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), 0);
-    return self->cfile.uoffset;
+    return self->cfile->uoffset;
 }
 
 G_GNUC_INTERNAL guint32
 gcab_file_get_usize (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), 0);
-    return self->cfile.usize;
+    return self->cfile->usize;
 }
 
 G_GNUC_INTERNAL GFile *
@@ -210,14 +212,14 @@ G_GNUC_INTERNAL void
 gcab_file_add_attribute (GCabFile *self, guint32 attribute)
 {
     g_return_if_fail (GCAB_IS_FILE (self));
-    self->cfile.fattr |= attribute;
+    self->cfile->fattr |= attribute;
 }
 
 G_GNUC_INTERNAL cfile_t *
 gcab_file_get_cfile (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), NULL);
-    return &self->cfile;
+    return self->cfile;
 }
 
 /**
@@ -234,7 +236,7 @@ gcab_file_get_size (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), 0);
 
-    return self->cfile.usize;
+    return self->cfile->usize;
 }
 
 /**
@@ -255,8 +257,8 @@ gcab_file_get_date (GCabFile *self, GTimeVal *tv)
     g_return_if_fail (GCAB_IS_FILE (self));
     g_return_if_fail (tv != NULL);
 
-    date = self->cfile.date;
-    time = self->cfile.time;
+    date = self->cfile->date;
+    time = self->cfile->time;
 
     tm.tm_isdst = -1;
     tm.tm_year  = ((date >> 9) + 1980) - 1900;
@@ -285,7 +287,7 @@ gcab_file_get_attributes (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), 0);
 
-    return self->cfile.fattr;
+    return self->cfile->fattr;
 }
 
 /**
@@ -301,7 +303,7 @@ gcab_file_get_name (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), NULL);
 
-    return self->cfile.name;
+    return self->cfile->name;
 }
 
 /**
@@ -341,19 +343,21 @@ gcab_file_new_with_file (const gchar *name, GFile *file)
     g_return_val_if_fail (name != NULL, NULL);
     g_return_val_if_fail (G_IS_FILE (file), NULL);
 
-    return g_object_new (GCAB_TYPE_FILE,
-                         "name", name,
-                         "file", file,
-                         NULL);
+    GCabFile *self = g_object_new (GCAB_TYPE_FILE,
+                                   "file", file,
+                                   NULL);
+    self->cfile = g_new0 (cfile_t, 1);
+    gcab_file_set_name (self, name);
+    return self;
 }
 
 G_GNUC_INTERNAL GCabFile *
-gcab_file_new_with_cfile (const cfile_t *cfile)
+gcab_file_new_steal_cfile (cfile_t **cfile)
 {
     g_return_val_if_fail (cfile != NULL, NULL);
 
     GCabFile *file = g_object_new (GCAB_TYPE_FILE, NULL);
-    file->cfile = *cfile;
+    file->cfile = g_steal_pointer (cfile);
 
     return file;
 }
@@ -371,7 +375,7 @@ gcab_file_get_extract_name (GCabFile *self)
 {
     g_return_val_if_fail (GCAB_IS_FILE (self), NULL);
 
-    return self->extract_name ? self->extract_name : self->cfile.name;
+    return self->extract_name ? self->extract_name : self->cfile->name;
 }
 
 /**
diff --git a/libgcab/gcab-folder.c b/libgcab/gcab-folder.c
index 7785fda..fc9b389 100644
--- a/libgcab/gcab-folder.c
+++ b/libgcab/gcab-folder.c
@@ -53,7 +53,7 @@ struct _GCabFolder
     GHashTable *hash;
     gint comptype;
     GByteArray *reserved;
-    cfolder_t cfolder;
+    cfolder_t *cfolder;
     GInputStream *stream;
 };
 
@@ -79,6 +79,7 @@ gcab_folder_finalize (GObject *object)
 {
     GCabFolder *self = GCAB_FOLDER (object);
 
+    cfolder_free (self->cfolder);
     g_slist_free_full (self->files, g_object_unref);
     g_hash_table_unref (self->hash);
     if (self->reserved)
@@ -328,13 +329,16 @@ gcab_folder_new (gint comptype)
 }
 
 G_GNUC_INTERNAL GCabFolder *
-gcab_folder_new_with_cfolder (const cfolder_t *folder, GInputStream *stream)
+gcab_folder_new_steal_cfolder (cfolder_t **cfolder, GInputStream *stream)
 {
+    g_return_val_if_fail (cfolder != NULL, NULL);
+    g_return_val_if_fail (G_IS_INPUT_STREAM (stream), NULL);
+
     GCabFolder *self = g_object_new (GCAB_TYPE_FOLDER,
-                                     "comptype", folder->typecomp,
+                                     "comptype", (*cfolder)->typecomp,
                                      NULL);
     self->stream = g_object_ref (stream);
-    self->cfolder = *folder;
+    self->cfolder = g_steal_pointer (cfolder);
 
     return self;
 }
@@ -383,11 +387,14 @@ gcab_folder_extract (GCabFolder *self,
     cdata_t cdata = { 0, };
     guint32 nubytes = 0;
 
+    /* never loaded from a stream */
+    g_assert (self->cfolder != NULL);
+
     data = g_data_input_stream_new (self->stream);
     g_data_input_stream_set_byte_order (data, G_DATA_STREAM_BYTE_ORDER_LITTLE_ENDIAN);
     g_filter_input_stream_set_close_base_stream (G_FILTER_INPUT_STREAM (data), FALSE);
 
-    if (!g_seekable_seek (G_SEEKABLE (data), self->cfolder.offsetdata, G_SEEK_SET, cancellable, error))
+    if (!g_seekable_seek (G_SEEKABLE (data), self->cfolder->offsetdata, G_SEEK_SET, cancellable, error))
         goto end;
 
     files = g_slist_sort (g_slist_copy (self->files), (GCompareFunc)sort_by_offset);
@@ -440,7 +447,7 @@ gcab_folder_extract (GCabFolder *self,
 
         /* let's rewind if need be */
         if (uoffset < nubytes) {
-            if (!g_seekable_seek (G_SEEKABLE (data), self->cfolder.offsetdata,
+            if (!g_seekable_seek (G_SEEKABLE (data), self->cfolder->offsetdata,
                                   G_SEEK_SET, cancellable, error))
                 goto end;
             bzero(&cdata, sizeof(cdata));
diff --git a/libgcab/gcab-priv.h b/libgcab/gcab-priv.h
index 6052a43..0cde13d 100644
--- a/libgcab/gcab-priv.h
+++ b/libgcab/gcab-priv.h
@@ -40,8 +40,8 @@
                                           _GCAB_GET (data, 1, 32,  8) |  \
                                           _GCAB_GET (data, 0, 32,  0))
 
-GCabFolder *     gcab_folder_new_with_cfolder        (const cfolder_t *folder, GInputStream *stream);
-GCabFile *       gcab_file_new_with_cfile            (const cfile_t *file);
+GCabFolder *     gcab_folder_new_steal_cfolder       (cfolder_t **cfolder, GInputStream *stream);
+GCabFile *       gcab_file_new_steal_cfile           (cfile_t   **cfile);
 
 gboolean         gcab_file_update_info               (GCabFile *file, GFileInfo *info);
 guint32          gcab_file_get_uoffset               (GCabFile *file);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]