[Pkg-xfce-devel] Bug#519181: xfce4-sensors-plugin: Found error causing the crash

[dawitbro at sbcglobal.net (Dave Witbrodt)]

Package: xfce4-sensors-plugin
Version: 0.10.99.6-1
Followup-For: Bug #519181


In  xfce4-sensors-plugin-0.10.99.6/lib  there is a file  'hddtemp.c'.  It
is in this file that the plugin is crashing for me.  (This is visible in
the output of the gdb 'where' command in my last post.)

The location of the crash in is the function called get_hddtemp_value(),
which begins at line 434.  In the variable declarations for this function,
this is declared near the beginning:

    #ifndef HAVE_LIBNOTIFY
	gchar *checktext;
    #endif

Near the end of the same function, we have a series of calls to g_free():

	g_free (cmd_line);
	g_free (standard_output);
	g_free (standard_error);
	g_free (msg_text);
    #ifndef HAVE_LIBNOTIFY
	g_free (checktext);
    #endif

The error is that not all code paths define a value for "checktext", so
that random undefined values of that variable are being passed to g_free()
when the plugin runs on my system.  Here are the actual lines of the
get_hddtemp_value() function that execute on my system:

    double
    get_hddtemp_value (char* disk, gboolean *suppressmessage)
    {
	gchar *standard_output, *standard_error;
	gchar *cmd_line, *msg_text;
    #ifndef HAVE_LIBNOTIFY
	gchar *checktext;
    #endif
	gint exit_status=0;
	double value;
	gboolean result, nevershowagain;
	GError *error;

	gchar *tmp, *tmp2, *tmp3;

	if (suppressmessage!=NULL)
	    nevershowagain = *suppressmessage;
    [...]

	error = NULL;
	cmd_line = g_strdup_printf ( "%s -n -q %s", PATH_HDDTEMP, disk);
	result = g_spawn_command_line_sync ( (const gchar*) cmd_line,
		&standard_output, &standard_error, &exit_status, &error);
    #endif

	msg_text = NULL; // wonder if this is still necessary

	DBG ("Exit code %d on %s with stdout of %s.\n", exit_status, disk, standard_output);

	/* filter those with no sensors out */
	if (exit_status==0 && strncmp(disk, "/dev/fd", 6)==0) { /* is returned for floppy disks */

    [...]

	else if ((exit_status==256 || (standard_error && strlen(standard_error)>0))
		&& access (PATH_HDDTEMP, X_OK)==0) /* || strlen(standard_error)>0) */

    [...]

	else if (error && (!result || exit_status!=0))

    [...]

	else if (standard_output && strlen(standard_output) > 0)
	{
	    DBG("got the only useful return value of 0 and value of %s.\n", standard_output);
	    /* hddtemp does not return floating values, but only integer ones.
	      So have an easier life with atoi.
	      FIXME: Use strtod() instead?*/
	    value = (double) (atoi ( (const char*) standard_output) );
	}

    [...]

	g_free (cmd_line);
	g_free (standard_output);
	g_free (standard_error);
	g_free (msg_text);
    #ifndef HAVE_LIBNOTIFY
	g_free (checktext);
    #endif

For me, this code path is followed MOST often:

  - I have no floppy disk devices, so the first "if" condition is always
    false

  - when g_spawn_command_line_sync() runs, it (usually) runs
    successfully... meaning exit_status == 0 and standard_error points to
    an empty string... so the first "else if" condition is false

  - also because g_spawn_command_line_sync() runs successfully, we get
    error == 0... so the second "else if" condition is false

  - at this point, standard_output points to the string for the successful
    shell exit code, "0"... so the third "else if" condition is true, and
    checktext remains undefined

When g_free (checktext) is encountered, chances are that it will be trying
to free a meaningless or illegal value.  With 'gdb', I found that the call
to get_hddtemp_value() does not fail every time.  I did not inspect the
reason why very carefully:  it could be because sometimes some other code
path is being followed, or it could be that some of the undefined (random)
values for checktext are treated as legitimate (!) by g_free().

In any case, this is definitely a coding error.  I would suggest moving the
declaration for checktext into the code blocks where it is used, and
freeing it at the end of those blocks.


HTH,
Dave W.

-- System Information:
Debian Release: squeeze/sid
  APT prefers unstable
  APT policy: (990, 'unstable'), (350, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 2.6.28-2s13073.090316.desktop.uvesafb (SMP w/2 CPU cores; PREEMPT)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/bash

Versions of packages xfce4-sensors-plugin depends on:
ii  libatk1.0-0                  1.24.0-2    The ATK accessibility toolkit
ii  libc6                        2.9-6       GNU C Library: Shared libraries
ii  libcairo2                    1.8.6-2+b1  The Cairo 2D vector graphics libra
ii  libfontconfig1               2.6.0-3     generic font configuration library
ii  libfreetype6                 2.3.9-4     FreeType 2 font engine, shared lib
ii  libglib2.0-0                 2.20.0-2    The GLib library of C routines
ii  libgtk2.0-0                  2.14.7-4+b1 The GTK+ graphical user interface 
ii  libpango1.0-0                1.22.4-2    Layout and rendering of internatio
ii  libsensors4                  1:3.1.0-2   library to read temperature/voltag
ii  libxfce4util4                4.4.2-3     Utility functions library for Xfce
ii  libxfcegui4-4                4.4.2-4     Basic GUI C functions for Xfce4
ii  xfce4-panel                  4.4.2-6     The Xfce4 desktop environment pane

Versions of packages xfce4-sensors-plugin recommends:
ii  hddtemp                    0.3-beta15-45 hard drive temperature monitoring 
ii  lm-sensors                 1:3.1.0-2     utilities to read temperature/volt

Versions of packages xfce4-sensors-plugin suggests:
pn  xsensors                      <none>     (no description available)

-- no debconf information