BUG in chkmail() - incorrect check for padvance() stop condition

BUG in chkmail() - incorrect check for padvance() stop condition

[Denys Vlasenko]

chkmail(void)
{
         ...
         for (mtp = mailtime; mtp < mailtime + MAXMBOXES; mtp++) {
                 int len;

                 len = padvance_magic(&mpath, nullstr, 2);
                 if (!len)  <================================== WRONG, should be "len < 0"
                         break;
                 p = stackblock();
                 if (*p == '\0')
                         continue;
                 for (q = p ; *q ; q++);
                 q[-1] = '\0';
		...

The bug is not noticed only because after MAXMBOXES iterations,
we exit the for() loop:

newfstatat(AT_FDCWD, "/var/spool/mail/user", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/mail/use", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/mail/us", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/mail/u", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/mail/", {st_mode=S_IFDIR|0775, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/var/spool/mail", {st_mode=S_IFDIR|0775, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/var/spool/mai", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/ma", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/m", 0x7ffd08acd420, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/spool/", {st_mode=S_IFDIR|0755, st_size=4096, ...}, 0) = 0
write(2, "$ ", 2# )                       = 2
read(0, "", 1)

and no visible problem is seen.

Re: BUG in chkmail() - incorrect check for padvance() stop condition

[Harald van Dijk]

On 03/04/2023 13:39, Denys Vlasenko wrote:
> chkmail(void)
> {
>          ...
>          for (mtp = mailtime; mtp < mailtime + MAXMBOXES; mtp++) {
>                  int len;
> 
>                  len = padvance_magic(&mpath, nullstr, 2);
>                  if (!len)  <================================== WRONG, 
> should be "len < 0"

You are correct. A patch for this has been submitted, 
<https://patchwork.kernel.org/project/dash/patch/20211002203732.6175-1-june@causal.agency/>, 
but is still pending.

Cheers,
Harald van Dijk