Date: Sat, 21 Jun 2014 07:59:41 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Stefan Farfeleder <stefanf@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r267675 - head/lib/libc/regex Message-ID: <20140621072634.I921@besplex.bde.org> In-Reply-To: <53A48D62.4060801@FreeBSD.org> References: <201406201529.s5KFTAEB068038@svn.freebsd.org> <20140620182311.GA1214@mole.fafoe.narf.at> <53A48D62.4060801@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> El 6/20/2014 1:23 PM, Stefan Farfeleder escribi=F3:
>> On Fri, Jun 20, 2014 at 03:29:10PM +0000, Pedro F. Giffuni wrote:
>>> ...
>>> Log:
>>> regex: Make use of reallocf().
>>>=20
>>> Use of reallocf is useful in libraries as we are not certain the
>>> application will exit after NULL.
>>> ...
>>> This somewhat reduces portability but if since you are building
>>> this as part of libc it is likely you have our non-standard
>>> reallocf(3) already.
It also somewhat increases namespace pollution.
>>> Modified: head/lib/libc/regex/regcomp.c
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
>>> --- head/lib/libc/regex/regcomp.c=09Fri Jun 20 13:26:49 2014=20
>>> (r267674)
>>> +++ head/lib/libc/regex/regcomp.c=09Fri Jun 20 15:29:09 2014=20
>>> (r267675)
>>> @@ -1111,7 +1111,7 @@ allocset(struct parse *p)
>>> {
>>> =09cset *cs, *ncs;
>>>=20
>>> -=09ncs =3D realloc(p->g->sets, (p->g->ncsets + 1) * sizeof(*ncs));
>>> +=09ncs =3D reallocf(p->g->sets, (p->g->ncsets + 1) * sizeof(*ncs));
>>> =09if (ncs =3D=3D NULL) {
>>> =09=09SETERROR(REG_ESPACE);
>>> =09=09return (NULL);
>>> @@ -1174,7 +1174,7 @@ CHadd(struct parse *p, cset *cs, wint_t
>>> =09if (ch < NC)
>>> =09=09cs->bmp[ch >> 3] |=3D 1 << (ch & 7);
>>> =09else {
>>> -=09=09newwides =3D realloc(cs->wides, (cs->nwides + 1) *
>>> +=09=09newwides =3D reallocf(cs->wides, (cs->nwides + 1) *
>>> =09=09 sizeof(*cs->wides));
>>> =09=09if (newwides =3D=3D NULL) {
>>> =09=09=09SETERROR(REG_ESPACE);
>>=20
>> I don't think these changes are OK. If reallocf() fails here, the
>> cs->wides pointer will be freed and later freeset() will call
>> free(cs->wides), probably crashing. The other cases are most probably
>> similar though I haven't examined them closely.
>>=20
>
> OK ...
>
> I don't think there is any problem:
>
> If reallocf fails, newwides will be set to NULL and if free() is called i=
t=20
> doesn't do anything when the argument is NULL.
>
> Also freeset() is meant to be called to "free a now-unused set" and it is=
not=20
> called within the library. I would think using a value when the allocatio=
n=20
> has failed is a much more serious issue than attempting to fail to free i=
t=20
> after trying to use it. ;-).
It doesn't look OK to me. It turns the careful use of newwides, etc.,
into garbage, and leaves a garbage pointer in cs->wides, etc., to cause
problems later. It might work without this careful use of a temporary
variable, but even that is not clear. Consider:
- start with a consistent data structure with some pointers to malloced
storage in it; foo->p say
- simple reallocf() allocation strategy:
foo->p =3D reallocf(foo->p, size)
if (foo->p =3D=3D NULL)
return (ERROR);
This leaves the data structure consistent if and only if the only thing
in it relevant to p is p itself, with foo->p serving as a flag for
validity.
- more complicated reallocf() allocation strategy:
foo->p =3D reallocf(foo->p, size)
if (foo->p =3D=3D NULL) {
foo->psize =3D 0;
foo->pvalid =3D 0;
foo->foovalid =3D 0;
...
return (ERROR);
}
This still can't do things like cleaning up what foo->p points to, since
reallocf() clobbered that.
This shows that reallocf() is only useful in simple cases. In general, you
must keep the pointer valid to clean things up:
newp =3D reallocf(foo->p, size)
if (newp =3D=3D NULL) {
for (i =3D 0; i < foo->psize; i++)
=09free(foo->p[i]);
foo->p =3D NULL;
foo->psize =3D 0;
foo->pvalid =3D 0;
foo->foovalid =3D 0;
...
return (ERROR);
}
Of course, malloc never fails so all this error checking is more a source
of complexity and bugs than useful.
Re-quoting/recovering some context:
@ Modified: head/lib/libc/regex/regcomp.c
@ =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
@ --- head/lib/libc/regex/regcomp.c=09Fri Jun 20 13:26:49 2014=09(r267674)
@ +++ head/lib/libc/regex/regcomp.c=09Fri Jun 20 15:29:09 2014=09(r267675)
@ @@ -1111,7 +1111,7 @@ allocset(struct parse *p)
@ {
@ =09cset *cs, *ncs;
@=20
@ -=09ncs =3D realloc(p->g->sets, (p->g->ncsets + 1) * sizeof(*ncs));
@ +=09ncs =3D reallocf(p->g->sets, (p->g->ncsets + 1) * sizeof(*ncs));
@ =09if (ncs =3D=3D NULL) {
@ =09=09SETERROR(REG_ESPACE);
@ =09=09return (NULL);
This code shows that even Henry didn't know how to program memory allocatio=
n
in 1988. The code was much worse in 4.4BSD-Lite2:
old@ =09=09if (p->g->sets =3D=3D NULL)
old@ =09=09=09p->g->sets =3D (cset *)malloc(nc * sizeof(cset));
old@ =09=09else
old@ =09=09=09p->g->sets =3D (cset *)realloc((char *)p->g->sets,
old@ =09=09=09=09=09=09=09nc * sizeof(cset));
This just leaked memory. It was improved by assigning to ncs and by
removing the pre-C90 and C++ casts. Now it is unimproved by turning the
careful use of ncs into garbage and leaving garbage in *p.
old@ =09=09if (p->g->setbits =3D=3D NULL)
old@ =09=09=09p->g->setbits =3D (uch *)malloc(nbytes);
old@ =09=09else {
old@ =09=09=09p->g->setbits =3D (uch *)realloc((char *)p->g->setbits,
old@ =09=09=09=09=09=09=09=09nbytes);
old@ =09=09=09/* xxx this isn't right if setbits is now NULL */
old@ =09=09=09for (i =3D 0; i < no; i++)
old@ =09=09=09=09p->g->sets[i].ptr =3D p->g->setbits + css*(i/CHAR_BIT);
old@ =09=09}
Honest memory mismanagement. It just assumes that malloc() and realloc()
can't fail. In -current, the function is much simpler and doesn't have
this code. I think part of the simplication is to use realloc() of NULL
instead of malloc() to start up.
@ @@ -1174,7 +1174,7 @@ CHadd(struct parse *p, cset *cs, wint_t=20
@ =09if (ch < NC)
@ =09=09cs->bmp[ch >> 3] |=3D 1 << (ch & 7);
@ =09else {
@ -=09=09newwides =3D realloc(cs->wides, (cs->nwides + 1) *
@ +=09=09newwides =3D reallocf(cs->wides, (cs->nwides + 1) *
@ =09=09 sizeof(*cs->wides));
@ =09=09if (newwides =3D=3D NULL) {
@ =09=09=09SETERROR(REG_ESPACE);
@ @@ -1203,7 +1203,7 @@ CHaddrange(struct parse *p, cset *cs, wi
@ =09=09CHadd(p, cs, min);
@ =09if (min >=3D max)
@ =09=09return;
@ -=09newranges =3D realloc(cs->ranges, (cs->nranges + 1) *
@ +=09newranges =3D reallocf(cs->ranges, (cs->nranges + 1) *
@ =09 sizeof(*cs->ranges));
@ =09if (newranges =3D=3D NULL) {
@ =09=09SETERROR(REG_ESPACE);
@ @@ -1227,7 +1227,7 @@ CHaddtype(struct parse *p, cset *cs, wct
@ =09for (i =3D 0; i < NC; i++)
@ =09=09if (iswctype(i, wct))
@ =09=09=09CHadd(p, cs, i);
@ -=09newtypes =3D realloc(cs->types, (cs->ntypes + 1) *
@ +=09newtypes =3D reallocf(cs->types, (cs->ntypes + 1) *
@ =09 sizeof(*cs->types));
@ =09if (newtypes =3D=3D NULL) {
@ =09=09SETERROR(REG_ESPACE);
@ @@ -1350,7 +1350,7 @@ enlarge(struct parse *p, sopno size)
@ =09if (p->ssize >=3D size)
@ =09=09return 1;
@=20
@ -=09sp =3D (sop *)realloc(p->strip, size*sizeof(sop));
@ +=09sp =3D (sop *)reallocf(p->strip, size*sizeof(sop));
@ =09if (sp =3D=3D NULL) {
@ =09=09SETERROR(REG_ESPACE);
@ =09=09return 0;
Similarly in 4 more cases, except most of them weren't in old versions.
@ @@ -1368,7 +1368,7 @@ static void
@ stripsnug(struct parse *p, struct re_guts *g)
@ {
@ =09g->nstates =3D p->slen;
@ -=09g->strip =3D (sop *)realloc((char *)p->strip, p->slen * sizeof(sop));
@ +=09g->strip =3D (sop *)reallocf((char *)p->strip, p->slen * sizeof(sop))=
;
@ =09if (g->strip =3D=3D NULL) {
@ =09=09SETERROR(REG_ESPACE);
@ =09=09g->strip =3D p->strip;
This was the only realloc() that had not been cleaned up, so using
reallocf() has a chance of improving it. It still has the pre-C90 and
C++ casts. But there is an obvious new bug visible without reading more
than the patch context:
- the local variable might not be needed now, since the variable assigned
to, g->strip, is different from the variable realloced, p->strip
- on realloc failure, p->strip becomes invalid
- the error handling is completely broken. It points g->strip at the old
(now deallocated) storage. This is immediate use of the garbage pointer
created by using reallocf().
Clearly, only realloc() code of the form "p =3D realloc(p, size);", where t=
he
pointer assigned to is the same as the pointer realloced, can be improved
by blindly converting it to use reallocf().
The last function is most interesting. It seems to be to reduce the size.
So an allocation failure is even less likely than usually, except lots of
small allocations and reallocations are more likely to waste space than
save it. But if failure occurs it is almost harmless, and the error
handling of keeping using the previous allocation was good. Maybe Henry
knew how to program memory allocation after all. (The last function
survived previously-unchanged from 4.4BSDLite-2 except for removing K&R
support and 'register'.)
Bruce
From owner-svn-src-head@FreeBSD.ORG Sat Jun 21 00:53:57 2014
Return-Path: <owner-svn-src-head@FreeBSD.ORG>
Delivered-To: svn-src-head@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115])
(using TLSv1 with cipher ADH-AES256-SHA (256/256 bits))
(No client certificate requested)
by hub.freebsd.org (Postfix) with ESMTPS id 8C422155;
Sat, 21 Jun 2014 00:53:57 +0000 (UTC)
Received: from svn.freebsd.org (svn.freebsd.org
[IPv6:2001:1900:2254:2068::e6a:0])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client did not present a certificate)
by mx1.freebsd.org (Postfix) with ESMTPS id 5E8FA2CC7;
Sat, 21 Jun 2014 00:53:57 +0000 (UTC)
Received: from svn.freebsd.org ([127.0.1.70])
by svn.freebsd.org (8.14.8/8.14.8) with ESMTP id s5L0rvKk038938;
Sat, 21 Jun 2014 00:53:57 GMT
(envelope-from jhibbits@svn.freebsd.org)
Received: (from jhibbits@localhost)
by svn.freebsd.org (8.14.8/8.14.8/Submit) id s5L0rvH7038936;
Sat, 21 Jun 2014 00:53:57 GMT
(envelope-from jhibbits@svn.freebsd.org)
Message-Id: <201406210053.s5L0rvH7038936@svn.freebsd.org>
From: Justin Hibbits <jhibbits@FreeBSD.org>
Date: Sat, 21 Jun 2014 00:53:57 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-head@freebsd.org
Subject: svn commit: r267697 - head/sys/dev/adb
X-SVN-Group: head
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-head@freebsd.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: SVN commit messages for the src tree for head/-current
<svn-src-head.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-head>,
<mailto:svn-src-head-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>;
List-Post: <mailto:svn-src-head@freebsd.org>
List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-head>,
<mailto:svn-src-head-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 21 Jun 2014 00:53:57 -0000
Author: jhibbits
Date: Sat Jun 21 00:53:56 2014
New Revision: 267697
URL: http://svnweb.freebsd.org/changeset/base/267697
Log:
No need to check if devd is running before posting an event.
Modified:
head/sys/dev/adb/adb_buttons.c
head/sys/dev/adb/adb_kbd.c
Modified: head/sys/dev/adb/adb_buttons.c
==============================================================================
--- head/sys/dev/adb/adb_buttons.c Sat Jun 21 00:45:44 2014 (r267696)
+++ head/sys/dev/adb/adb_buttons.c Sat Jun 21 00:53:56 2014 (r267697)
@@ -118,37 +118,32 @@ abtn_receive_packet(device_t dev, u_char
switch (cmd) {
case 0x0a: /* decrease brightness */
- if (devctl_process_running())
- devctl_notify("PMU", "keys", "brightness",
- "notify=down");
+ devctl_notify("PMU", "keys", "brightness",
+ "notify=down");
break;
case 0x09: /* increase brightness */
- if (devctl_process_running())
- devctl_notify("PMU", "keys", "brightness", "notify=up");
+ devctl_notify("PMU", "keys", "brightness", "notify=up");
break;
case 0x08: /* mute */
case 0x01: /* mute, AV hardware */
- if (devctl_process_running())
- devctl_notify("PMU", "keys", "mute", NULL);
+ devctl_notify("PMU", "keys", "mute", NULL);
break;
case 0x07: /* decrease volume */
case 0x02: /* decrease volume, AV hardware */
- if (devctl_process_running())
- devctl_notify("PMU", "keys", "volume", "notify=down");
+ devctl_notify("PMU", "keys", "volume", "notify=down");
break;
case 0x06: /* increase volume */
case 0x03: /* increase volume, AV hardware */
- if (devctl_process_running())
- devctl_notify("PMU", "keys", "volume", "notify=up");
+ devctl_notify("PMU", "keys", "volume", "notify=up");
break;
case 0x0c: /* mirror display key */
/* Need callback to do something with this */
break;
case 0x0b: /* eject tray */
- if (devctl_process_running())
- devctl_notify("PMU", "keys", "eject", NULL);
+ devctl_notify("PMU", "keys", "eject", NULL);
+ break;
case 0x7f: /* numlock */
/* Need callback to do something with this */
break;
Modified: head/sys/dev/adb/adb_kbd.c
==============================================================================
--- head/sys/dev/adb/adb_kbd.c Sat Jun 21 00:45:44 2014 (r267696)
+++ head/sys/dev/adb/adb_kbd.c Sat Jun 21 00:53:56 2014 (r267697)
@@ -424,7 +424,7 @@ adb_kbd_receive_packet(device_t dev, u_c
mtx_lock(&sc->sc_mutex);
/* 0x7f is always the power button */
- if (data[0] == 0x7f && devctl_process_running()) {
+ if (data[0] == 0x7f) {
devctl_notify("PMU", "Button", "pressed", NULL);
mtx_unlock(&sc->sc_mutex);
return (0);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140621072634.I921>