[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230330135205.GA4163@brightrain.aerifal.cx>
Date: Thu, 30 Mar 2023 09:52:06 -0400
From: Rich Felker <dalias@...c.org>
To: Matthias Goergens <matthias.goergens@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] Fix UB in getmntent_r on extremely long lines
On Thu, Mar 30, 2023 at 04:12:20PM +0800, Matthias Goergens wrote:
> 8974ef2124118e4ed8cad7ee0534b36e5c584c4e tried to fix mishandling of
> extremely long lines.
>
> Here's the relevant code snippet:
>
> ```
> len = strlen(linebuf);
> if (len > INT_MAX) continue;
> for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> sscanf(linebuf, " %n%*s%n %n%*s%n %n%*s%n %n%*s%n %d %d",
> n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> &mnt->mnt_freq, &mnt->mnt_passno);
> } while (linebuf[n[0]] == '#' || n[1]==len);
> ```
>
> Alas, that introduced undefined behaviour: if the very first line
> handled in the function is extremely long, `n` stays uninitialised, and
> thus accessing `n[0]` and `n[1]` is UB.
>
> If we handle a few sane lines before hitting a crazy long line, we don't
> hit C-level undefined behaviour, but the function arguably still does
> the wrong thing.
>
> The man page says:
>
> > The getmntent() and getmntent_r() functions return a pointer to the
> > mntent structure or NULL on failure.
>
> So this patch does exactly that: return NULL to inform the caller that
> an error occured.
> ---
> src/misc/mntent.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> index d404fbe3..d91c4964 100644
> --- a/src/misc/mntent.c
> +++ b/src/misc/mntent.c
> @@ -43,7 +43,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> }
>
> len = strlen(linebuf);
> - if (len > INT_MAX) continue;
> + if (len > INT_MAX) return NULL;
> for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> sscanf(linebuf, " %n%*s%n %n%*s%n %n%*s%n %n%*s%n %d %d",
> n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> --
> 2.40.0
This changes the intended behavior of the function, causing it to
error out on an (invalid entry) long line rather than skipping it and
continuing to the next.
Since the loop condition is not valid to evaluate in this case, the
simplest fix is probably replacing continue with goto and a label at
the start of the loop.
Rich
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.
