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