NetBSD Problem Report #44888

From www@NetBSD.org  Wed Apr 20 22:42:41 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id E95FA63C2A1
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 20 Apr 2011 22:42:40 +0000 (UTC)
Message-Id: <20110420224239.C0F5F63C226@www.NetBSD.org>
Date: Wed, 20 Apr 2011 22:42:39 +0000 (UTC)
From: lwindschuh@gmail.com
Reply-To: lwindschuh@gmail.com
To: gnats-bugs@NetBSD.org
Subject: [patch] libedit read.c: fix logic
X-Send-Pr-Version: www-1.0

>Number:         44888
>Category:       lib
>Synopsis:       [patch] libedit read.c: fix logic
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Apr 20 22:45:00 +0000 2011
>Last-Modified:  Wed Apr 20 23:10:03 +0000 2011
>Originator:     Lucius Windschuh
>Release:        FreeBSD 9-CURRENT
>Organization:
>Environment:
does not apply, using libedit on FreeBSD
>Description:
wpa_supplicant segfaults on my FreeBSD machine since NetBSD's libedit readline support was imported.

Digging deeper, libedit's el_push in read.c looked suspicious (here in FreeBSD's version, but with the same logic):



public void

el_push(EditLine *el, const char *str)

{

  c_macro_t *ma = &el->el_chared.c_macro;



  if (str != NULL && ma->level + 1 < EL_MAXMACRO) {

    ma->level++;

    if ((ma->macro[ma->level] = el_strdup(str)) != NULL)

      return;

    ma->level--;

  }

  term_beep(el);

  term__flush();

}



It is strange to me that ma->level is not permanently incremented, only in the edge case where strdup returns NULL (which happens in my case).


So, I propose the fix blow, which not only fixes the segfaults for me, but also makes the logic consistent to me:
ma->level is incremented iff strdup() succeeds, reflecting the macro level change.

The fix to the NetBSD code is similar, the bug exists since read.c version 1.30.
>How-To-Repeat:
How to reproduce (at least on FreeBSD with enabled malloc debugging): 

$ wpa_cli

> disco

> reass

(wait until more output is generated)

(press enter)

-> segfault


Backtrace from gdb:
(gdb)  bt
#0  0x281f05f7 in kill () from /lib/libc.so.7
#1  0x281f0516 in raise () from /lib/libc.so.7
#2  0x281eefca in abort () from /lib/libc.so.7
#3  0x281704f3 in malloc_usable_size () from /lib/libc.so.7
#4  0x28171fda in malloc_usable_size () from /lib/libc.so.7
#5  0x28172f5a in free () from /lib/libc.so.7
#6  0x280aaf02 in read_pop (ma=0xbfbfe22c) at read.c:311
#7  0x280adb41 in el_getc (el=0x2844e300, 
    cp=0xbfbfe49b "ZD&#65533;\v(<&#65533;\v(\002\201D(&#65533;&#65533;&#65533;\017\203\n(") at read.c:345
#8  0x280b704f in el_gets (el=0x2844e300, nread=0xbfbfe4c8) at read.c:239
#9  0x280a830f in readline (p=0x805002a "> ") at /usr/src/lib/libedit/readline.c:404
#10 0x0804c4de in main (argc=Error accessing memory address 0x5: Bad address.
)
    at /usr/src/usr.sbin/wpa/wpa_cli/../../../contrib/wpa//wpa_supplicant/wpa_cli.c:2038

>Fix:
Index: read.c

===================================================================

--- read.c      (Revision 220736)

+++ read.c      (Arbeitskopie)

@@ -217,9 +217,10 @@



        if (str != NULL && ma->level + 1 < EL_MAXMACRO) {

                ma->level++;

-               if ((ma->macro[ma->level] = el_strdup(str)) != NULL)

+               if ((ma->macro[ma->level] = el_strdup(str)) != NULL) {

+                       ma->level--;

                        return;

-               ma->level--;

+               }

        }

        term_beep(el);

        term__flush();

>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, lib-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: lib/44888: [patch] libedit read.c: fix logic
Date: Wed, 20 Apr 2011 18:52:33 -0400

 On Apr 20, 10:45pm, lwindschuh@gmail.com (lwindschuh@gmail.com) wrote:
 -- Subject: lib/44888: [patch] libedit read.c: fix logic

 | It is strange to me that ma->level is not permanently incremented, only in the edge case where strdup returns NULL (which happens in my case).

 It is permanently increament when strdup does not return NULL. In the error
 case where strdup returns NULL it is decremented. Perhaps something else
 is going on?

 christos

From: Lucius Windschuh <lwindschuh@googlemail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/44888: [patch] libedit read.c: fix logic
Date: Thu, 21 Apr 2011 01:08:23 +0200

 2011/4/21 Christos Zoulas <christos@zoulas.com>:
 > =A0On Apr 20, 10:45pm, lwindschuh@gmail.com (lwindschuh@gmail.com) wrote:
 > =A0-- Subject: lib/44888: [patch] libedit read.c: fix logic
 >
 > =A0| It is strange to me that ma->level is not permanently incremented, o=
 nly in the edge case where strdup returns NULL (which happens in my case).
 >
 > =A0It is permanently increament when strdup does not return NULL. In the =
 error
 > =A0case where strdup returns NULL it is decremented. Perhaps something el=
 se
 > =A0is going on?

 I'm sorry, you are totally right. I threw up =3D=3D NULL and !=3D NULL. :-(

 Lucius

Home	PR Database Search