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
Description: PGP signature
