[musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[raf]

Hi,

fnmatch() enters an infinite loop if the pattern is the non-character byte
'\xf8'. This patch fixes it by stepping over the invalid byte.

cheers,
raf

[musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[raf]

From: raf <raf@raf.org>

---
 src/regex/fnmatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/regex/fnmatch.c b/src/regex/fnmatch.c
index 978fff88..59f94d9f 100644
--- a/src/regex/fnmatch.c
+++ b/src/regex/fnmatch.c
@@ -89,7 +89,7 @@ escaped:
 		wchar_t wc;
 		int k = mbtowc(&wc, pat, m);
 		if (k<0) {
-			*step = 0;
+			*step = 1;
 			return UNMATCHABLE;
 		}
 		*step = k + esc;
-- 
2.39.5

Re: [musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[Rich Felker]

On Sat, Oct 04, 2025 at 10:54:13PM +1000, raf wrote:
> Hi,
> 
> fnmatch() enters an infinite loop if the pattern is the non-character byte
> '\xf8'. This patch fixes it by stepping over the invalid byte.

This does not sound like a correct fix. The UNMATCHABLE return from
pat_next should produce an immediate FNM_NOMATCH return, not resume
parsing in an invalid location.

For future submissions, could you also please include patch
descriptions in the same email as the patch itself rather than
degenerate 2-mail threads? That helps make it so we can reply to both
the description/motivation and the actual code changes in a single
reply mail without manually copy-pasting from multiple mails and
picking a place to stitch it into the thread.

Rich

Re: [musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[raf]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On Mon, Oct 06, 2025 at 09:40:36PM -0400, Rich Felker <dalias@libc.org> wrote:

> On Sat, Oct 04, 2025 at 10:54:13PM +1000, raf wrote:
> > Hi,
> > 
> > fnmatch() enters an infinite loop if the pattern is the non-character byte
> > '\xf8'. This patch fixes it by stepping over the invalid byte.
> 
> This does not sound like a correct fix. The UNMATCHABLE return from
> pat_next should produce an immediate FNM_NOMATCH return, not resume
> parsing in an invalid location.

There are 2 calls to pat_next where the UNMATCHABLE return value
results in the caller returning FNM_NOMATCH. These calls are in
fnmatch_internal().

There are 2 more calls to pat_next() in fnmatch_internal() and 1
more in fnmatch() where the caller doesn't check for UNMATCHABLE
and then return FNM_NOMATCH.

Attached is a patch that adds the 3 missing checks for UNMATCHABLE.

> For future submissions, could you also please include patch
> descriptions in the same email as the patch itself rather than
> degenerate 2-mail threads? That helps make it so we can reply to both
> the description/motivation and the actual code changes in a single
> reply mail without manually copy-pasting from multiple mails and
> picking a place to stitch it into the thread.

Sure. I was using git send-email. I won't do that anymore.

> Rich

cheers,
raf


[-- Attachment #2: 0001-fnmatch-fix-infinite-loop-when-pattern-is-non-charac.patch --]
[-- Type: text/x-diff, Size: 1593 bytes --]

From 9aa072b4e5029ec6dc030e2add20855e95182511 Mon Sep 17 00:00:00 2001
From: raf <raf@raf.org>
Date: Wed, 8 Oct 2025 12:55:58 +1100
Subject: [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

---
 src/regex/fnmatch.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/regex/fnmatch.c b/src/regex/fnmatch.c
index 978fff88..1bbcf28e 100644
--- a/src/regex/fnmatch.c
+++ b/src/regex/fnmatch.c
@@ -240,6 +240,8 @@ static int fnmatch_internal(const char *pat, size_t m, const char *str, size_t n
 	p = ptail;
 	for (;;) {
 		c = pat_next(p, endpat-p, &pinc, flags);
+		if (c == UNMATCHABLE)
+			return FNM_NOMATCH;
 		p += pinc;
 		if ((k = str_next(s, endstr-s, &sinc)) <= 0) {
 			if (c != END) return FNM_NOMATCH;
@@ -265,6 +267,8 @@ static int fnmatch_internal(const char *pat, size_t m, const char *str, size_t n
 		s = str;
 		for (;;) {
 			c = pat_next(p, endpat-p, &pinc, flags);
+			if (c == UNMATCHABLE)
+				return FNM_NOMATCH;
 			p += pinc;
 			/* Encountering * completes/commits a component */
 			if (c == STAR) {
@@ -302,7 +306,9 @@ int fnmatch(const char *pat, const char *str, int flags)
 	int c;
 	if (flags & FNM_PATHNAME) for (;;) {
 		for (s=str; *s && *s!='/'; s++);
-		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!='/'; p+=inc);
+		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!=UNMATCHABLE && c!='/'; p+=inc);
+		if (c == UNMATCHABLE)
+			return FNM_NOMATCH;
 		if (c!=*s && (!*s || !(flags & FNM_LEADING_DIR)))
 			return FNM_NOMATCH;
 		if (fnmatch_internal(pat, p-pat, str, s-str, flags))
-- 
2.50.0

Re: [musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[Thorsten Glaser]

On Wed, 8 Oct 2025, raf wrote:

>-		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!='/'; p+=inc);
>+		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!=UNMATCHABLE && c!='/'; p+=inc);
>+		if (c == UNMATCHABLE)
>+			return FNM_NOMATCH;

This would have been a *wonderful* chance to clean up that absolute
footgun of a loop construct (the semicolon even trailing and with
no comment):

		for (p = pat; (c = pat_next(p, -1, &inc, flags)) != END && c != '/'; p += inc)
			if (c == UNMATCHABLE)
				return (FNM_NOMATCH);

Just my impression on casual looking at that diff (from outside),
//mirabilos
-- 
Sometimes they [people] care too much: pretty printers [and syntax highligh-
ting, d.A.] mechanically produce pretty output that accentuates irrelevant
detail in the program, which is as sensible as putting all the prepositions
in English text in bold font.	-- Rob Pike in "Notes on Programming in C"

Re: [musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[raf]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On Wed, Oct 08, 2025 at 12:56:49PM +1100, raf <musl@raf.org> wrote:

> On Mon, Oct 06, 2025 at 09:40:36PM -0400, Rich Felker <dalias@libc.org> wrote:
> 
> > On Sat, Oct 04, 2025 at 10:54:13PM +1000, raf wrote:
> > > Hi,
> > > 
> > > fnmatch() enters an infinite loop if the pattern is the non-character byte
> > > '\xf8'. This patch fixes it by stepping over the invalid byte.
> > 
> > This does not sound like a correct fix. The UNMATCHABLE return from
> > pat_next should produce an immediate FNM_NOMATCH return, not resume
> > parsing in an invalid location.
> 
> There are 2 calls to pat_next where the UNMATCHABLE return value
> results in the caller returning FNM_NOMATCH. These calls are in
> fnmatch_internal().
> 
> There are 2 more calls to pat_next() in fnmatch_internal() and 1
> more in fnmatch() where the caller doesn't check for UNMATCHABLE
> and then return FNM_NOMATCH.
> 
> Attached is a patch that adds the 3 missing checks for UNMATCHABLE.
> 
> > For future submissions, could you also please include patch
> > descriptions in the same email as the patch itself rather than
> > degenerate 2-mail threads? That helps make it so we can reply to both
> > the description/motivation and the actual code changes in a single
> > reply mail without manually copy-pasting from multiple mails and
> > picking a place to stitch it into the thread.
> 
> Sure. I was using git send-email. I won't do that anymore.
> 
> > Rich
> 
> cheers,
> raf

Perhaps it's only the call to pat_next() in fnmatch() that needed
to be changed. There is a comment in fnmat_internal() explaining
why the last two calls to pat_next() did not require checking for
UNMATCHABLE:

	/* Past this point we need not check for UNMATCHABLE in pat,
	 * because all of pat has already been parsed once. */

Attach is an alternative patch that only adds one check for UNMATCHABLE
in fnmatch() if you'd prefer that.

cheers,
raf


[-- Attachment #2: 0001-fnmatch-fix-infinite-loop-when-pattern-is-non-charac.patch --]
[-- Type: text/x-diff, Size: 953 bytes --]

From 751514ea4622584b2e390e96ca38fcd069408cff Mon Sep 17 00:00:00 2001
From: raf <raf@raf.org>
Date: Wed, 8 Oct 2025 12:55:58 +1100
Subject: [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

---
 src/regex/fnmatch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/regex/fnmatch.c b/src/regex/fnmatch.c
index 978fff88..d3ee699f 100644
--- a/src/regex/fnmatch.c
+++ b/src/regex/fnmatch.c
@@ -302,7 +302,9 @@ int fnmatch(const char *pat, const char *str, int flags)
 	int c;
 	if (flags & FNM_PATHNAME) for (;;) {
 		for (s=str; *s && *s!='/'; s++);
-		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!='/'; p+=inc);
+		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!=UNMATCHABLE && c!='/'; p+=inc);
+		if (c == UNMATCHABLE)
+			return FNM_NOMATCH;
 		if (c!=*s && (!*s || !(flags & FNM_LEADING_DIR)))
 			return FNM_NOMATCH;
 		if (fnmatch_internal(pat, p-pat, str, s-str, flags))
-- 
2.50.0

Re: [musl] [PATCH] fnmatch: fix infinite loop when pattern is non-character byte

[raf]

On Wed, Oct 08, 2025 at 05:04:48AM +0200, Thorsten Glaser <tg@mirbsd.de> wrote:

> On Wed, 8 Oct 2025, raf wrote:
> 
> >-		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!='/'; p+=inc);
> >+		for (p=pat; (c=pat_next(p, -1, &inc, flags))!=END && c!=UNMATCHABLE && c!='/'; p+=inc);
> >+		if (c == UNMATCHABLE)
> >+			return FNM_NOMATCH;
> 
> This would have been a *wonderful* chance to clean up that absolute
> footgun of a loop construct (the semicolon even trailing and with
> no comment):
> 
> 		for (p = pat; (c = pat_next(p, -1, &inc, flags)) != END && c != '/'; p += inc)
> 			if (c == UNMATCHABLE)
> 				return (FNM_NOMATCH);
> 
> Just my impression on casual looking at that diff (from outside),
> //mirabilos

I don't think so. That formatting is used in many
places in this code. It's fine. Changing a single
instance would introduce an inconsistency which I think
would be detrimental. But more importantly, it's
presumably Rich's preferred formatting. It fits more
code per screen. While I don't personally appreciate
the benefit in that myself, I'm not going to argue with
it. It's not my project. Any anyway, people who are
susceptible to footguns usually don't choose to
implement the entire C library. :-)

And the code looks surprisingly pleasant to me.

cheers,
raf