Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 101104
Summary: | crash in Gpm_Open | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Retired] Red Hat Linux Beta | Reporter: | Geoff Reedy <geoff+fedora> | ||||||||||||
Component: | gpm | Assignee: | Eido Inoue <havill> | ||||||||||||
Status: | CLOSED RAWHIDE | QA Contact: | David Lawrence <dkl> | ||||||||||||
Severity: | medium | Docs Contact: | |||||||||||||
Priority: | medium | ||||||||||||||
Version: | beta1 | CC: | aoliva, leonard-rh-bugzilla | ||||||||||||
Target Milestone: | --- | ||||||||||||||
Target Release: | --- | ||||||||||||||
Hardware: | i386 | ||||||||||||||
OS: | Linux | ||||||||||||||
Whiteboard: | |||||||||||||||
Fixed In Version: | 1.20.1-38 | Doc Type: | Bug Fix | ||||||||||||
Doc Text: | Story Points: | --- | |||||||||||||
Clone Of: | Environment: | ||||||||||||||
Last Closed: | 2003-08-07 17:48:34 UTC | Type: | --- | ||||||||||||
Regression: | --- | Mount Type: | --- | ||||||||||||
Documentation: | --- | CRM: | |||||||||||||
Verified Versions: | Category: | --- | |||||||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||||||
Embargoed: | |||||||||||||||
Bug Depends On: | |||||||||||||||
Bug Blocks: | 100644 | ||||||||||||||
Attachments: |
|
Description
Geoff Reedy
2003-07-29 03:46:50 UTC
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. |