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-all@FreeBSD.ORG Sat Jun 21 00:30:52 2014
Return-Path: <owner-svn-src-all@FreeBSD.ORG>
Delivered-To: svn-src-all@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 3AC3C986;
Sat, 21 Jun 2014 00:30:52 +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 249842B46;
Sat, 21 Jun 2014 00:30:52 +0000 (UTC)
Received: from svn.freebsd.org ([127.0.1.70])
by svn.freebsd.org (8.14.8/8.14.8) with ESMTP id s5L0UqMs028471;
Sat, 21 Jun 2014 00:30:52 GMT (envelope-from np@svn.freebsd.org)
Received: (from np@localhost)
by svn.freebsd.org (8.14.8/8.14.8/Submit) id s5L0UpOL028469;
Sat, 21 Jun 2014 00:30:51 GMT (envelope-from np@svn.freebsd.org)
Message-Id: <201406210030.s5L0UpOL028469@svn.freebsd.org>
From: Navdeep Parhar <np@FreeBSD.org>
Date: Sat, 21 Jun 2014 00:30:51 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org
Subject: svn commit: r267694 - stable/10/sys/dev/cxgbe
X-SVN-Group: stable-10
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for "
user" and " projects" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>;
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 21 Jun 2014 00:30:52 -0000
Author: np
Date: Sat Jun 21 00:30:51 2014
New Revision: 267694
URL: http://svnweb.freebsd.org/changeset/base/267694
Log:
MFC r267600:
cxgbe(4): Fix bug in the fast rx buffer recycle path. In some cases rx
buffers were getting recycled when they should have been left alone.
Modified:
stable/10/sys/dev/cxgbe/adapter.h
stable/10/sys/dev/cxgbe/t4_sge.c
Directory Properties:
stable/10/ (props changed)
Modified: stable/10/sys/dev/cxgbe/adapter.h
==============================================================================
--- stable/10/sys/dev/cxgbe/adapter.h Fri Jun 20 21:53:50 2014 (r267693)
+++ stable/10/sys/dev/cxgbe/adapter.h Sat Jun 21 00:30:51 2014 (r267694)
@@ -254,7 +254,8 @@ struct cluster_metadata {
struct fl_sdesc {
caddr_t cl;
- uint8_t nmbuf;
+ uint8_t nimbuf; /* # of inline mbufs with ref on the cluster */
+ uint8_t nembuf; /* # of allocated mbufs with ref */
struct cluster_layout cll;
};
Modified: stable/10/sys/dev/cxgbe/t4_sge.c
==============================================================================
--- stable/10/sys/dev/cxgbe/t4_sge.c Fri Jun 20 21:53:50 2014 (r267693)
+++ stable/10/sys/dev/cxgbe/t4_sge.c Sat Jun 21 00:30:51 2014 (r267694)
@@ -1498,22 +1498,22 @@ get_scatter_segment(struct adapter *sc,
/* copy data to mbuf */
bcopy(payload, mtod(m, caddr_t), len);
- } else if (sd->nmbuf * MSIZE < cll->region1) {
+ } else if (sd->nimbuf * MSIZE < cll->region1) {
/*
* There's spare room in the cluster for an mbuf. Create one
- * and associate it with the payload that's in the cluster too.
+ * and associate it with the payload that's in the cluster.
*/
MPASS(clm != NULL);
- m = (struct mbuf *)(sd->cl + sd->nmbuf * MSIZE);
+ m = (struct mbuf *)(sd->cl + sd->nimbuf * MSIZE);
/* No bzero required */
if (m_init(m, NULL, 0, M_NOWAIT, MT_DATA, flags | M_NOFREE))
return (NULL);
fl->mbuf_inlined++;
m_extaddref(m, payload, padded_len, &clm->refcount, rxb_free,
swz->zone, sd->cl);
- sd->nmbuf++;
+ sd->nimbuf++;
} else {
@@ -1527,10 +1527,11 @@ get_scatter_segment(struct adapter *sc,
if (m == NULL)
return (NULL);
fl->mbuf_allocated++;
- if (clm != NULL)
+ if (clm != NULL) {
m_extaddref(m, payload, padded_len, &clm->refcount,
rxb_free, swz->zone, sd->cl);
- else {
+ sd->nembuf++;
+ } else {
m_cljset(m, sd->cl, swz->type);
sd->cl = NULL; /* consumed, not a recycle candidate */
}
@@ -3053,7 +3054,7 @@ refill_fl(struct adapter *sc, struct sge
if (sd->cl != NULL) {
- if (sd->nmbuf == 0) {
+ if (sd->nimbuf + sd->nembuf == 0) {
/*
* Fast recycle without involving any atomics on
* the cluster's metadata (if the cluster has
@@ -3062,6 +3063,11 @@ refill_fl(struct adapter *sc, struct sge
* fit within a single mbuf each.
*/
fl->cl_fast_recycled++;
+#ifdef INVARIANTS
+ clm = cl_metadata(sc, fl, &sd->cll, sd->cl);
+ if (clm != NULL)
+ MPASS(clm->refcount == 1);
+#endif
goto recycled_fast;
}
@@ -3107,7 +3113,8 @@ recycled:
#endif
clm->refcount = 1;
}
- sd->nmbuf = 0;
+ sd->nimbuf = 0;
+ sd->nembuf = 0;
recycled_fast:
fl->pending++;
fl->needed--;
@@ -3176,7 +3183,7 @@ free_fl_sdesc(struct adapter *sc, struct
cll = &sd->cll;
clm = cl_metadata(sc, fl, cll, sd->cl);
- if (sd->nmbuf == 0 ||
+ if (sd->nimbuf + sd->nembuf == 0 ||
(clm && atomic_fetchadd_int(&clm->refcount, -1) == 1)) {
uma_zfree(sc->sge.sw_zone_info[cll->zidx].zone, sd->cl);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140621072634.I921>