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

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 18 Jun 2026 14:46:50 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<ajPophf-WlcQmJHC@devuan>
On 2026-06-18T14:43:52+0200, Alejandro Colomar wrote:
> 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"

Actually, I meant strcspn().

And I forgot to declare len as size_t.

> 	+  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>



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