[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20260909172554.GI23438@brightrain.aerifal.cx>
Date: Wed, 9 Sep 2026 13:25:55 -0400
From: Rich Felker <dalias@...c.org>
To: Matthias Goergens <matthias.goergens@...il.com>,
musl@...ts.openwall.com
Subject: Re: Re: [PATCH v2] regex: avoid overflow sizing glob result
vector
On Wed, Aug 05, 2026 at 04:07:37PM +0200, Szabolcs Nagy wrote:
> * Matthias Goergens <matthias.goergens@...il.com> [2026-08-05 21:47:48 +0800]:
> > GLOB_DOOFFS makes gl_offs part of the caller-controlled result-vector
> > size. Unchecked allocation arithmetic can wrap to a small buffer, then
> > out-of-bounds pointer writes follow. GLOB_APPEND has the same issue.
> >
> > Validate the complete pointer count, including its terminating null,
> > before either allocation. This also makes later indices and the count
> > update safe while preserving an existing result on append failure.
> > ---
> > v2: simplified the overflow check per Szabolcs Nagy's suggestion;
> > kept the explicit guard for caller-controlled gl_offs.
>
> i thought gl_offs > lim was a user error
gl_offs is requesting that many slots at the beginning. Requesting
more than can fit in PTRDIFF_MAX is a guaranteed to fail, but it's not
undefined, so we need to ensure that the value doesn't cause a false
success via overflow.
> fwiw if we must check it i'd do it like
>
> if ((offs|n) > -1/sizeof(char*)/2) ...
I suppose this is very mildly more computationally efficient, but I'm
not sure the non-obviousness is worth it. This is in no way a
bottleneck.
> > src/regex/glob.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/regex/glob.c b/src/regex/glob.c
> > index 87bae084..6488ebfe 100644
> > --- a/src/regex/glob.c
> > +++ b/src/regex/glob.c
> > @@ -269,8 +269,19 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
> > return GLOB_NOMATCH;
> > }
> >
> > + /* offs is caller-controlled with GLOB_DOOFFS, so it needs an
> > + * explicit guard. gl_pathc was bounded by this same check on the
> > + * call that produced it, and cnt live match allocations cannot
> > + * reach lim, so the sum below cannot wrap. */
> > + size_t lim = -1/sizeof(char *)/2;
> > + size_t n = offs + g->gl_pathc + cnt + 1;
> > + if (offs >= lim || n > lim) {
> > + freelist(&head);
> > + return GLOB_NOSPACE;
> > + }
Specifically, if gl_pathc is nonzero (if GLOB_APPEND is set),
offs+g->gl_pathc+1 is already known to be the existing dimension of
the allocated array. Only cnt could make it overflow, and in fact that
too is impossible:
Each match string takes at least 1 byte of string data and at least
sizeof(struct match) bytes of bookkeeping, not counting allocator
overhead (which in reality is at least that amount again). Since all
of these fit in address space (SIZE_MAX), cnt can be at most
SIZE_MAX/(1+sizeof(char*)).
This means the addition offs + g->gl_pathc + cnt + 1 can never
overflow (in append case). What about the mul? It's borderline.
We know (offs+g->gl_pathc+1)*sizeof(char*) <= PTRDIFF_MAX. From the
above bound, we only have cnt*sizeof(char*)<K*SIZE_MAX, where K is 4/5
(32-bit) or 8/9 (64-bit). This is not enough to preclude overflow, but
would be if you could allocator overhead which makes cnt at most
something like SIZE_MAX/16.
Thus, in the append case, there is no real-world possibility of
overflow, and the theoretical overflow is precluded just by checking
n<=lim or cnt<=lim.
In the no-append case, offs isn't known to be a realizable array
dimension, so it needs to be checked before anything can be added to
it. Like in the append case, as long as offs<=lim, adding cnt cannot
overflow, so we're free to check either n<=lim or cnt<=lim.
As such, I think the above works, and I don't think there's any way to
do much better than it without assuming the allocator overhead keeps
cnt small enough (in which case only offs needs checking).
It might be marginally nicer to check:
+ if (offs > lim || cnt > lim) {
just so the person reading the logic doesn't have to reason that the
addition to compute n didn't overflow. I don't think it makes any
functional difference though. This would also avoid the need for the
temp variable n and the diff hitting lines that don't really need
anything changed:
> > +
> > if (flags & GLOB_APPEND) {
> > - char **pathv = realloc(g->gl_pathv, (offs + g->gl_pathc + cnt + 1) * sizeof(char *));
> > + char **pathv = realloc(g->gl_pathv, n * sizeof(char *));
> > if (!pathv) {
> > freelist(&head);
> > return GLOB_NOSPACE;
> > @@ -278,7 +289,7 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
> > g->gl_pathv = pathv;
> > offs += g->gl_pathc;
> > } else {
> > - g->gl_pathv = malloc((offs + cnt + 1) * sizeof(char *));
> > + g->gl_pathv = malloc(n * sizeof(char *));
> > if (!g->gl_pathv) {
> > freelist(&head);
> > return GLOB_NOSPACE;
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
