After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 646926 - arg_data invalid after g_option_context_parse() fails
arg_data invalid after g_option_context_parse() fails
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.28.x
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 770078
 
 
Reported: 2011-04-06 15:29 UTC by Jan Kneschke
Modified: 2016-09-13 02:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test-case for g_option_context_parse() (1.25 KB, text/plain)
2011-04-06 15:29 UTC, Jan Kneschke
  Details
patch: goption: Don't return pointers to deallocated memory (1.60 KB, patch)
2016-08-31 17:16 UTC, Kjell Ahlstedt
committed Details | Review

Description Jan Kneschke 2011-04-06 15:29:25 UTC
Created attachment 185322 [details]
Test-case for g_option_context_parse()

If a option is specified twice on the commandline and a unknown option is specified like:

  $ prg --known=foo --known=bar --unknown=baz

the arg_data of the "known" option isn't left unchanged. Instead it contains a pointer to free()d memory:

==53227== Invalid free() / delete / delete[]
==53227==    at 0x10000FC7C: free (vg_replace_malloc.c:366)
==53227==    by 0x100000DC1: main (goption-cli.c:46)
==53227==  Address 0x1056dd870 is 0 bytes inside a block of size 7 free'd
==53227==    at 0x10000FC7C: free (vg_replace_malloc.c:366)
==53227==    by 0x10005A623: parse_arg (in /opt/local/lib/libglib-2.0.0.dylib)
==53227==    by 0x10005AA56: parse_long_option (in /opt/local/lib/libglib-2.0.0.dylib)
==53227==    by 0x10005B553: g_option_context_parse (in /opt/local/lib/libglib-2.0.0.dylib)
==53227==    by 0x100000D77: main (goption-cli.c:34)

The attached testcase shows the faulty behaviour.

  $ gcc -Wall -O2 -g `pkg-config glib-2.0 --libs --cflags` goption-cli.c -o goption-cli 
  $ valgrind goption-cli

The problem is in goption.c: parse_arg() for G_OPTION_ARG_FILENAME that does:

 /* first value */
 data = g_strdup (value);
 change = get_change (context, G_OPTION_ARG_FILENAME, entry->arg_data);
 change->prev.str = *(gchar **)entry->arg_data;   /* sets the original value */
 g_free (change->allocated.str); /* noop */
 change->allocated.str = data;
 *(gchar **)entry->arg_data = data;

 /* second value */
 data = g_strdup (value);
 change = get_change (context, G_OPTION_ARG_FILENAME, entry->arg_data); /* returns the same "Change" */
 change->prev.str = *(gchar **)entry->arg_data;      /* points to the 'data' of the previous round */
 g_free (change->allocated.str);       /* BUG: free's the previous data, makes change->prev.str pointing to a invalid location */
 change->allocated.str = data;
 *(gchar **)entry->arg_data = data;

in free_changes_list() it does:

 g_free (change->allocated.str);
 *(gchar **)change->arg_data = change->prev.str; /* change->prev.str is already invalid */

G_OPTION_ARG_STRING has the same problem.

Instead of setting change->prev.str each time, it should only be set for the first time which change is used. A possible fix could be:

--- glib-2.28.5/glib/goption.c  2011-03-04 18:50:39.000000000 +0100
+++ glib-2.28.5-goption-bug/glib/goption.c      2011-04-06 16:19:04.000000000 +0200
@@ -1212,9 +1212,12 @@
 #endif
         change = get_change (context, G_OPTION_ARG_FILENAME,
                              entry->arg_data);
+       if (NULL == change->allocated.str) {
+          /* only store the original value of arg_data */
+          change->prev.str = *(gchar **)entry->arg_data;
+        }
         g_free (change->allocated.str);
 
-        change->prev.str = *(gchar **)entry->arg_data;
         change->allocated.str = data;
 
         *(gchar **)entry->arg_data = data;
Comment 1 Kjell Ahlstedt 2016-08-31 17:16:50 UTC
Created attachment 334545 [details] [review]
patch: goption: Don't return pointers to deallocated memory

This is a git patch with Jan Kneschke's fix.
Comment 2 Kjell Ahlstedt 2016-09-09 15:23:54 UTC
Hello, glib maintainers! This is a bug that can cause a program crash. (Not
obvious from the description or the selected importance, I admit.) How come
it has not got any attention for 5 years? The suggested fix is simple.
Can I push the patch?