Bug 101104 - crash in Gpm_Open
crash in Gpm_Open
Status: CLOSED RAWHIDE
Product: Red Hat Linux Beta
Classification: Retired
Component: gpm (Show other bugs)
beta1
i386 Linux
medium Severity medium
: ---
: ---
Assigned To: Eido Inoue
David Lawrence
:
: 100836 (view as bug list)
Depends On:
Blocks: CambridgeTarget
  Show dependency treegraph
 
Reported: 2003-07-28 23:46 EDT by Geoff Reedy
Modified: 2007-04-18 12:56 EDT (History)
2 users (show)

See Also:
Fixed In Version: 1.20.1-38
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2003-08-07 13:48:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Backtrace of crash in vim (634 bytes, text/plain)
2003-07-28 23:48 EDT, Geoff Reedy
no flags Details
simple program demonstrating the bug (181 bytes, text/plain)
2003-08-01 22:08 EDT, Geoff Reedy
no flags Details
patch against gpm-1.20.1-35 (621 bytes, patch)
2003-08-01 22:12 EDT, Geoff Reedy
no flags Details | Diff
Patch for Gpm_Open() (918 bytes, patch)
2003-08-03 12:21 EDT, Leonard den Ottolander
no flags Details | Diff
Spec file for gpm with gpmopen patch (12.58 KB, text/plain)
2003-08-03 12:25 EDT, Leonard den Ottolander
no flags Details

  None (edit)
Description Geoff Reedy 2003-07-28 23:46:50 EDT
Description of problem:

There is a NULL pointer dereference in Gpm_Open when a NULL pointer is passed to
strlen().  I have only observed this problem with VIM, but examining the code it
looks as though it couls affect other programs as well.

Version-Release number of selected component (if applicable):

1.20.1-35

How reproducible:

Easy

Steps to Reproduce:
1. Edit a read-only file with vim on the console.
2. As soon as vim starts to load hold down the 'd' key
    
Actual results:

Vim crashes with a segmentation fault, backtrace pending.

Expected results:

Vim behaves normally (all of the lines are deleted)

Additional info:

I will be attaching a backtrace and some more information about the cause of the
bug.
Comment 1 Geoff Reedy 2003-07-28 23:48:40 EDT
Created attachment 93212 [details]
Backtrace of crash in vim
Comment 2 Geoff Reedy 2003-07-28 23:56:37 EDT
Near the beginning of Gpm_Open (liblow.c:202) options.consolename is set to
NULL.  The pointer is filled with valid data at line 221, but it is guarded by a
conditional that lets that code execute only once.  If the function is called a
second time options.console name is once again set to NULL but the line that
sets options.consolename is skipped and the NULL pointer is passed to strlen at
line 263.  Possible fixes include guarding against a NULL pointer around line
263 or moving the options.consolename = NULL inside the conditional guarding
line 221.  Since I am not familiar with the intended meaning of the code I don't
know which if any of those solutions would be correct.
Comment 3 Eido Inoue 2003-07-29 10:51:34 EDT
fixed in release 36
Comment 4 Alexandre Oliva 2003-07-31 01:27:13 EDT
*** Bug 100836 has been marked as a duplicate of this bug. ***
Comment 5 Geoff Reedy 2003-08-01 22:05:50 EDT
This bug still persists.  It occurs anytime a program Gpm_Closes enough times
that libgpm's internal stack of connections objects is emptied.  This is
reproducible by a 10 line C program which I will attach to the bug.  I also have
a patch that fixes the bug completely and as far as I can tell preserves the
semantics of Gpm_Open/Close.
Comment 6 Geoff Reedy 2003-08-01 22:08:38 EDT
Created attachment 93353 [details]
simple program demonstrating the bug

This C program will trigger the bug, compile with
gcc -o gpmcrash gpmcrash.c -lgpm
Comment 7 Geoff Reedy 2003-08-01 22:12:03 EDT
Created attachment 93354 [details]
patch against gpm-1.20.1-35

This patch fixes the problem by only settings option.consolename to null when
it will be reassigned.	You can get rpms built with this patch from
http://web.umr.edu/~greedy/gpm/
Comment 8 Geoff Reedy 2003-08-01 22:33:57 EDT
An additional note on reproducing the bug in vim as initially reported, you must
have set mouse=a (or some other value that enables mouse support) in your .vimrc
file.
Comment 9 Leonard den Ottolander 2003-08-02 20:26:46 EDT
Hi Geoff,

Could you please add the SPEC file as well? This saves the time to download the
whole srpm.
Comment 10 Leonard den Ottolander 2003-08-02 22:03:34 EDT
For now this patch seems to work on my machine.

Also it looks sound: option.consolename should not be reset to NULL on every
call to Gpm_Open().

Is it actually necessary to set option.consolename to NULL before the call to
Gpm_get_console()?

The condition set in gpm-1.20.1-deref-null.patch doesn't seem to fix anything,
only avoid the strncmp if consolename is NULL. I don't think strncpm cares
whether option.consolename is NULL. Actually I think this is the condition where
Gpm_Open should fail.
Comment 11 Leonard den Ottolander 2003-08-02 22:53:06 EDT
> I don't think strncpm cares whether option.consolename is NULL.

 Missed that it is strlen that segfaults on this.

 But again, if option.consolename == NULL Gpm_Open should fail. Will
Gpm_get_console ever return NULL? Is the NULL consolename caught by
         if (tty == NULL) {
? 

If so, we probably only need the first hunk of Geoff's patch, not the
"deref-null" patch.

If not add a check whether option.consolename == NULL before the strncmp which
checks the consolename is valid. Something like

         if (tty == NULL) {
            gpm_report(GPM_PR_ERR,"checking tty name failed");
            goto err;
         }   
+         if (option.consolename == NULL) {
+            gpm_report(GPM_PR_ERR,"option.consolename is NULL");
+            goto err;
+         }
        /* do we really need this check ? */
         if(strncmp(tty,option.consolename,strlen(option.consolename)-1)
            || !isdigit(tty[strlen(option.consolename)-1])) {
            gpm_report(GPM_PR_ERR,"strncmp/isdigit/option.consolename failed");
            goto err;
         }
Comment 12 Geoff Reedy 2003-08-02 23:40:12 EDT
I agree that there ought to be something to somewhat gracefully handle the 
case where Gpm_get_console returns NULL, what you posted in your comment looks 
like a fine place to put it, basically it seems that if option.consolename is 
NULL it should have the same semantics of when the strcmp between tty and 
consolename fails.  Although I suspect it is a little late, I uploaded the 
patch and the spec file to the URL with the rpms, I will keep this in mind for 
the future though.
Comment 13 Leonard den Ottolander 2003-08-03 09:49:14 EDT
Note that the check whether consolename is NULL is only necessary if it could
happen that tty != NULL but option.consolename == NULL. Not sure if that is the
case.

Indeed I don't need the spec file anymore, but maybe you could attach it here
anyway?
Comment 14 Leonard den Ottolander 2003-08-03 12:16:44 EDT
The return value of Gpm_get_console() is set to NULL on initialisation. This
means option.consolename does not need to be explicitely set to NULL before
calling Gpm_get_console(). This means hunk #2 of Geoff's patch indeed can be
dropped.

The check whether option.consolename == NULL needs to be added and this should
be handled as an error condition.

This results in the following patch and spec file.
Comment 15 Leonard den Ottolander 2003-08-03 12:21:51 EDT
Created attachment 93366 [details]
Patch for Gpm_Open()

Removes the setting of option.consolename to NULL on every call of Gpm_Open().
Adds a check whether option.consolename is NULL and if so aborts.
Comment 16 Leonard den Ottolander 2003-08-03 12:25:00 EDT
Created attachment 93367 [details]
Spec file for gpm with gpmopen patch

Spec file to build gpm with gpmopen patch. Removes "deref-null" patch because
it is wrong. Spec file based on 1.20.1-36, so anything else added in -36 is
left intact.
Comment 17 Geoff Reedy 2003-08-06 23:38:00 EDT
Leonard's patch and spec file are working fine for me here.  Can we get them
pushed to rawhide (or the beta 2 queue if that's what's going on right now) so
this issue can get closed out?
Comment 18 Eido Inoue 2003-08-07 13:48:34 EDT
Looks good. Added in release 38. Thanks for the patch, Leonard.

Note You need to log in before you can comment on or make changes to this bug.