FreeBSD Port: qmail-1.03_3

[Matthias Andree]

Michael Sierchio <kudzu at tenebras.com> writes:

> You have added a patch which is totally unneccesary, and I
> suggest that it be removed:
>
> in your patch-qmail-local....
>
> -     while ((k > i) && (cmds.s[k - 1] == ' ') || (cmds.s[k - 1] == '\t'))
> +     while ((k > i) && ((cmds.s[k - 1] == ' ') || (cmds.s[k - 1] == '\t')))
>
> This is totally bogus, the original code is correct.

To clarify what we're talking about:

   645      {
   646       cmds.s[j] = 0;
   647       k = j;
>  648       while ((k > i) && (cmds.s[k - 1] == ' ') || (cmds.s[k - 1] == '\t'))
>  649         cmds.s[--k] = 0;
   650       switch(cmds.s[i])
   651        {

Your assertion is bogus, the original code is b0rked and has been
discussed on the qmail-list several times.

&& has a higher precedence than || (and if they had the same precedence,
it would not matter as these operators are left-to-right associative),
hence C will parse the original code on lines 648f. as

while (((k > i) && (cmds.s[k - 1] == ' '))
    || (cmds.s[k - 1] == '\t'))
    cmds.s[--k] = 0;

The intention however is 

while ((k > i)
    && ((cmds.s[k - 1] == ' ') || (cmds.s[k - 1] == '\t')))

(The parentheses around the relations might indeed be dropped however.)

Hence the patch is justified as it fixes this bug. The original code can
cause a buffer underrun, and, subsequently, a crash of the process.

AFAIR, this strikes on ~USER/.qmail* lines (or the first line)
consisting solely of TAB followed by an arbitrary combination of TAB and
SPACE characters. Not very common though, but happens occasionally
depending on the text editor used on those files.

> Please do not make changes to contributed code simply because it makes
> it easier for you to understand.

Please do not flame port maintainers for fixing bugs in upstream code
because *you* don't understand the code or the patch.

> There is no semantic difference
> between the two, ERGO the patch should not exist.  Please remove it.

Please leave the patch in for the sake of the poor souls who still use
this decrepit software.

> Shall I file a PR?

Please don't.  Instead, grab a good C book and read it.

Note that my defending the patch does not mean I endorse, recommend or
even suggest qmail. That decrepit piece of software has been
unmaintained with dozens of known bugs for the better part of a decade
now, is not safe to use on softupdates file systems, and it has a
considerable collection of other bugs, some of them deliberate violation
of standards (RFC-1652).

People ought to look at Postfix, Exim and Courier and perhaps a handful
of other MTAs before settling on qmail.

-- 
Matthias Andree