Re: Fwd: report 3/3: latent unterminated buffer in imap_wordcasecmp()

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 18 Jun 2026 14:43:47 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<ajPmK1WvfAfF4F2U@devuan>
Hi Kevin,

On 2026-06-18T18:25:01+0800, Kevin J. McCarthy wrote:
[...]
> > > @@ -888,7 +888,7 @@ int imap_wordcasecmp(const char *a, const char *b)
> > >    int i;
> > > 
> > >    tmp[SHORT_STRING-1] = 0;
> > 
> > This line seems to be redundant with the removed 'tmp[i+1] = 0;', IIUC.
> > So, it's fine to remove the one below, as you did.  Am I reading
> > correctly?
> 
> Yes, that's right.  It was redundant once the loop comparison was fixed.
> 
> I believe I alternatively could remove the
>   tmp[SHORT_STRING-1] = 0
> before the loop, along with the
>   tmp[i] = 0
> inside the loop, and change the assignment after the loop to
>   tmp[i] = 0
> I think the final assignment would cover both exit cases.
> 
> But, to me, it feels a little better seeing the first and middle
> assignments.  Otherwise I have to think more carefully about the two cases
> of i: one because of the "break" and the other because it failed the loop
> condition.

Okay.

> > > -  for (i=0;i < SHORT_STRING-2;i++,s++)
> > > +  for (i=0;i < SHORT_STRING-1;i++,s++)
> > 
> > Huh, why did we do -2 instead of -1?
> 
> Yes, this was the weird part.  I have no idea.  It's a straight-forward
> function, so I'm guessing it was just a think-o.  The only caller is looking
> at the CAPABILITY list coming from the IMAP server, word by word, and
> comparing it to the Capability array defined in imap/command.c.
> 
> The "bug" was if b was of length 127, the loop would terminate copying bytes
> 0..125.  The final for loop increment would set i=126,Then, "tmp[i+1] = 0"
> would nul-terminate the string at byte 127.  So byte 126 would be unset.
> 
> None of the capabilities we have in the mutt array are that long, so this
> wouldn't matter, but it's still a bug.

TBH, it still reads a bit too complex to me.  I struggle to fully
understand it.

I've tried rewriting the function from scratch, to see if I understand
it correctly.  Is this rewrite correct?

	@@ -884,20 +884,10 @@ void imap_unmunge_mbox_name(IMAP_DATA *idata, char *s)
	 int imap_wordcasecmp(const char *a, const char *b)
	 {
	   char tmp[SHORT_STRING];
	-  const char *s = b;
	-  int i;
	 
	-  tmp[SHORT_STRING-1] = 0;
	-  for (i=0;i < SHORT_STRING-2;i++,s++)
	-  {
	-    if (!*s || IS_ASCII_WS(*s))
	-    {
	-      tmp[i] = 0;
	-      break;
	-    }
	-    tmp[i] = *s;
	-  }
	-  tmp[i+1] = 0;
	+  len = strspn(b, " \t\n\v\f\r");  // We could #define ASCII_WS " \t\n\v\f\r"
	+  len = MIN(len, sizeof(tmp)-1);  // Is truncation a problem?!
	+  strcpy(mempcpy(tmp, b, len), "");
	 
	   return ascii_strcasecmp(a, tmp);
	 }

If this is correct, maybe it's less prone to such think-o's than the
hand-written loop?

Also, I still wonder what should happen on truncation.  If we've copied
a truncated word and ascii_strcasecmp() returns true, it might actually
be false?  Is that a latent bug too?


Cheers,
Alex

-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)