Re: fmttest(1) Suffers Segmentation Violation.

[Ralph Corderoy]

Hi Ken,

> > > Oh, strangely enough ... it's because it's trying to call
> > > seq_setprev() with no messages set.
>
> > Not for me as I don't have Previous-Sequence set.  git-grep(1)
> > suggests our test suite doesn't set it either?
>
> Well, geez, Ralph, you COULD run a debugger and figure out what's
> going wrong yourself! :-)

I did add some judicious printf()s before alerting the list.  :-)

> What is actually happening, for me at least, is that fmttest is
> calling seq_setprev(), which calls seq_addsel(), which ends up
> crashing in is_selected().  is_selected() is a macro which expands to:
>
>       bvector_at(msgstat(mp, msgnum), SELECTED)
>
> msgstat is also a macro which expands to:
>
>       ((mp)->msgstats + (n) - mp->lowoff)
>
> But in our case, msgnum is 0.  This causes a segfault because lowoff
> is 1, and this math means the pointer to msgstats is before the start
> of the msgstats array.  Why is msgnum 0?  Because of this loop:
>
>     for (msgnum = mp->lowsel; msgnum <= mp->hghsel; msgnum++)
>         if (is_selected (mp, msgnum))
>             add_sequence (mp, i, msgnum);
>
> The problem is in THIS case, mp->lowsel is zero (mp->hghsel is also
> zero).  Because of the <=, it means this loop is executed at least
> once.
>
> So, obviously, you're being tripped up by the same loop that appears
> further down in fmttest.c.

Yes, I worked that out from the printed debug which is why I said

   ‘I've had a look at the code and I suspect it's an easy fix related
    to spotting no messages have been provided before attempting to loop
    from 0 to 0 inclusive and indexing 0-1 but thought it better to leave
    the fix to someone more confident in what's meant to happen.  :-)’

The loop failing for me was a later one, like you say.

> Now, this makes me wonder what the REAL bug is.  This loop appears
> everywhere in nmh.  If there are no selected messages in a folder, how
> should this be represented in nmh?

struct msgs's has

    int lowsel;         /* Lowest selected msg number        */
    int hghsel;         /* Highest selected msg number       */
    int numsel;         /* Number of msgs selected           */

numsel is a guard against using lowsel and hghsel, just as

    int lowmsg;         /* Lowest msg number                 */
    int hghmsg;         /* Highest msg number                */
    int nummsg;         /* Actual Number of msgs             */

has nummsg to guard lowmsg and hghmsg.  That's what uip/sendsbr.c does:

    if (mp->numsel == 0) {
        if (debugsw)
            inform("no messages to annotate, continuing...");
        goto oops;
    }
    ...
    for (msgnum = mp->lowsel; msgnum <= mp->hghsel; msgnum++) {
        if (is_selected(mp, msgnum)) {
            if (debugsw)
                inform("annotate message %d", msgnum);
            annotate (m_name (msgnum), annotext, cp, inplace, 1, -2, 0);
        }
    }

> Right now when folder_read() is called and the folder structure is
> initialized, mp->lowsel is 0.  But mp->hghsel is also 0.  Should we
> initialize mp->hghsel to -1, so the loop never goes around? 

If it were [lowsel, highsel), i.e. a half-open interval, then 0,0 would
be an empty list.  Ditto lowmsg and highmsg.  The numsel and nummsg
wouldn't be needed.  Having high<low by using -1 seems a bit like two
wrongs not making a right.

Half-open intervals are nearly always a win, as you probably know but
the list might not.

    >>> s='abcdef'
    >>> def f(l, h): t=s[l:h]; print(len(t), repr(t))
    ...
    >>> f(0,0)
    0 ''
    >>> f(0,1)
    1 'a'
    >>> f(0,2)
    2 'ab'
    >>> f(1,1)
    0 ''
    >>> f(1,2)
    1 'b'
    >>> f(0,5)
    5 'abcde'
    >>> f(0,6)
    6 'abcdef'
    >>> f(0,len(s))
    6 'abcdef'
    >>>

Striding through an array is particularly nice as the low,high indexes
just shuffle along:  0,4 4,11 11,13 13,14

> Should we make it so is_selected() handles the case where it is called
> with an out-of-range message number?  (I mean, probably).

Having looked through the code, I don't think so.

- If a folder has messages 42-314 then the bit vector will use bit
  42-42=0 for 42, 314-42=272 for 314.

- Accessing bits before 0 isn't allowed by the vector routines so
  messages 1-41 shouldn't be looked up and there's no reason they should
  if numsel and nummsg are checked before lowsel or lowmsg are used.

- Message 0 isn't valid and should never be looked up.

- Messages below 0 are also invalid though possible given an int is
  usually used to hold the message number.  However, because the vector
  routines use size_t for the bit index, the negative will become a big
  positive allowing the problem to often go unnoticed, e.g. the routine
  bvector_at(vec, i) sees big i is way off the end of the storage so has
  never been set and returns its value is 0.

That's the vector code.  We could alter the next layer up which adjusts
the message number by lowoff, e.g. subtracts 42 because that's what
bit 0 represents.  Then, say, is_selected() would spot it's about to
pass a negative for size_t and call BUG(), aborting.

But I don't think these routines should workaround a wrong value; they
should highlight the faulty code.

-- 
Cheers, Ralph.