Re: GLib oddities

[Tim Janik <timj gtk org>]

On Fri, 19 May 2000, Tor Lillqvist wrote:

> A person who is building GLib with the Borland compiler (on Win32) has
> reported these oddities that the compiler emits warnings for.
> 
> The first does seem to be a real bug. In gmain.c:
> 
> static gboolean 
> g_idle_prepare  (gpointer  source_data, 
> 		 GTimeVal *current_time,
> 		 gint     *timeout,
> 		 gpointer  user_data)
> {
>   timeout = 0;		<<--- shouldn't this be *timeout = 0;?
>   return TRUE;
> }

right, though it's not a real bug because timeout is simply ignored
for prepare() calls that return TRUE (and automatically assumed 0).

> In gmem.c:g_mem_chunk_free(): He claims that g_tree_search() can
> return NULL, but still its return value isn't tested. Can
> g_tree_search() really return NULL in this case?  Would seem strange
> that nobody would have noticed this earlier.

well, users are supposed to pass correct memory pointers in to
g_mem_chunk_free(), in which case this code isn't triggered.
see, the code in question is:

g_mem_chunk_free (GMemChunk *mem_chunk, gpointer   mem) { [...]
  if (rmem_chunk->type == G_ALLOC_AND_FREE)
    {
      /* Place the memory on the "free_atoms" list */
      free_atom = (GFreeAtom*) mem;
      free_atom->next = rmem_chunk->free_atoms;
      rmem_chunk->free_atoms = free_atom;
      temp_area = g_tree_search (rmem_chunk->mem_tree,
                                 (GCompareFunc) g_mem_chunk_area_search,
                                 mem);
      temp_area->allocated -= 1;

you can trigger a segmentation fault in temp_area->allocated -= 1 with:

#include <glib.h>
int
main (int argc,
      char *argv[])
{
  GMemChunk *mc = g_mem_chunk_create (gpointer, 5, G_ALLOC_AND_FREE);
  gpointer dummy1[8], dummy2 = (gpointer) 42;

  g_mem_chunk_free (mc, dummy1);

  return 0;
}

but do s/dummy1/dummy2/ for the above testcode, and you get the
segmentation fault in free_atom->next = rmem_chunk->free_atoms
already.
so if at all, we should catch both cases, i.e. we can do:
  if (rmem_chunk->type == G_ALLOC_AND_FREE)
    {
      temp_area = g_tree_search (rmem_chunk->mem_tree,
                                 (GCompareFunc) g_mem_chunk_area_search,
                                 mem);
      if (!temp_area)
        g_error ("memchunk `%s': freeing of invalid address %p",
                 rmem_chunk->name, mem);

      /* Place the memory on the "free_atoms" list
       */
      free_atom = (GFreeAtom*) mem;
      free_atom->next = rmem_chunk->free_atoms;
      rmem_chunk->free_atoms = free_atom;

      temp_area->allocated -= 1;

if people prefer that over segmentation faults...

> 
> In garray.c, lots of unnecessary tests for unsigned values being >= 0...:
> 
> diff -u2 /src/glib/garray.c ./garray.c
> --- /src/glib/garray.c	Tue Apr 18 21:19:04 2000
> +++ ./garray.c	Fri May 12 23:19:14 2000
> @@ -210,5 +210,5 @@
>    g_return_val_if_fail (array, NULL);
>  
> -  g_return_val_if_fail (index >= 0 && index < array->len, NULL);
> +  g_return_val_if_fail (index < array->len, NULL);
[...]

applied.


> Unsigned types should be used in G_STRUCT_OFFSET and G_STRUCT_MEMBER_P?
> 
> diff -u2 /src/glib/glib.h ./glib.h
> @@ -191,7 +172,7 @@
>   */
>  #define G_STRUCT_OFFSET(struct_type, member)	\
> -    ((glong) ((guint8*) &((struct_type*) 0)->member))
> +    ((gulong) ((gchar*) &((struct_type*) 0)->member))
>  #define G_STRUCT_MEMBER_P(struct_p, struct_offset)   \
> -    ((gpointer) ((guint8*) (struct_p) + (glong) (struct_offset)))
> +    ((gpointer) ((gchar*) (struct_p) + (gulong) (struct_offset)))
>  #define G_STRUCT_MEMBER(member_type, struct_p, struct_offset)   \
>      (*(member_type*) G_STRUCT_MEMBER_P ((struct_p), (struct_offset)))

already in cvs.

> Some unnecessary assignments, initialisations and variables:
> 
> diff -u2 /src/glib/gscanner.c ./gscanner.c
> @@ -1627,5 +1626,4 @@
>  		  
>  		  gstring = g_string_append_c (gstring, ch);
> -		  ch = 0;
>  		}
>  	    }

done.

> @@ -1697,5 +1695,4 @@
>        value.v_string = gstring->str;
>        g_string_free (gstring, FALSE);
> -      gstring = NULL;
>      }

i'd rather leave this to trigger invalid gstring accesses after this
code portion for future alterations.

> 
> Cheers,
> --tml
> 

---
ciaoTJ