On Thu, Jun 18, 2026 at 11:01:52AM +0200, Alejandro Colomar via Mutt-dev wrote:
Hi Kevin, Kurt,On 2026-06-18T09:44:14+0800, Kevin J. McCarthy wrote:On Wed, Jun 17, 2026 at 09:01:01PM -0400, Kurt Hackenberg wrote: > On Thu, Jun 18, 2026 at 08:19 +0800, Kevin J. McCarthy wrote: > > > > Since Capabilities[x] elements are short fixed strings (e.g., > > > "IMAP4rev1", "CONDSTORE"), ascii_strcasecmp() will return before > > > reading the uninitialized tmp[126] byte. However, the helper is > > > buggy in isolation. > > > > This basically sums it up. The comparison function is buggy, but > > since mutt controls the list of compared strings we won't hit the > > bug. > > > > I don't want to accept an AI rewrite just before 2.4.0, so I'll try > > to look for alternative fixes. Or if any of you want to take a look > > and propose a minimal fix for before I get to it, that would be > > great too. > > All I know is your messages here, but offhand, I don't see a need for > any fix just before a release. A bug that won't happen can wait, can't > it? :-) Yes, that's a good point. I'll add a fix for this on to the future branch. Right now it looks like a simpler fix is just this, but I'll take a closer look after the 2.4.0 release. @@ -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.
- 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.
-- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
