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.
Created attachment 93212 [details] Backtrace of crash in vim
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.
fixed in release 36
*** Bug 100836 has been marked as a duplicate of this bug. ***
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.
Created attachment 93353 [details] simple program demonstrating the bug This C program will trigger the bug, compile with gcc -o gpmcrash gpmcrash.c -lgpm
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/
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.
Hi Geoff, Could you please add the SPEC file as well? This saves the time to download the whole srpm.
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.
> 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; }
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.
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?
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.
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.
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.
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?
Looks good. Added in release 38. Thanks for the patch, Leonard.