[musl] [PATCH] Fix UB in getmntent_r on extremely long lines

[musl] [PATCH] Fix UB in getmntent_r on extremely long lines

[Matthias Goergens]

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

Re: [musl] [PATCH] Fix UB in getmntent_r on extremely long lines

[Rich Felker]

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