From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 17555 invoked from network); 30 Mar 2023 13:52:23 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 30 Mar 2023 13:52:23 -0000 Received: (qmail 17947 invoked by uid 550); 30 Mar 2023 13:52:19 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 17900 invoked from network); 30 Mar 2023 13:52:18 -0000 Date: Thu, 30 Mar 2023 09:52:06 -0400 From: Rich Felker To: Matthias Goergens Cc: musl@lists.openwall.com Message-ID: <20230330135205.GA4163@brightrain.aerifal.cx> References: <20230330081220.1128115-1-matthias.goergens@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230330081220.1128115-1-matthias.goergens@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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