store: don't underflow pointer

[N-R-K]

when number of snips is 0, end - 1 will go one below the buffer. computing out of bound pointer (with the exception of one past the end) is technically UB [0] so avoid it by doing the computation on index instead of pointers.

0: https://port70.net/~nsz/c/c11/n1570.html#6.5.6p9

EDIT: the patch is incomplete, breaks reverse iteration.

EDIT2: turned out a bit more hairy than I had imagined, but the patch should be functional now. Just doing the same computation, but on index instead of pointer steers clear of any UB.

[cdown]

when number of snips is 0, end - 1 will go one below the buffer

Huh? end - 1 is still within the buffer, that's the header. Could you please show what you mean if not that?

[N-R-K]

I quoted the wrong part earlier (pointer-pointer subtraction instead of pointer-integer addition).

Here is the correct section and the relavant part (emphasis added):

If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.

The end - 1 does end up inside the mmap-ed buffer, but it's not the same cs_snip array.

Also relavant from annex j (it's talking about dereferencing, but the "out of range" part is relavant for pointer arithmetic also):

An array subscript is out of range, even if an object is apparently accessible with the given subscript (as in the lvalue expression a[1][7] given the declaration int a[4][5])

Doing the arithmetic on indices avoids all these issues.

[cdown]

Oh, I see. What do you think about just adjusting the original code with this in the *snip is NULL case:

if (start == end) {
    return false;
}

or

if (guard->cs->header->nr_snips == 0) {
    return false;
}

That seems to produce more easy to understand code flow to me.

[N-R-K]

That seems to produce more easy to understand code flow to me.

That's what I originally had but that only fixes the end - 1 computation. I realized later that decrement when doing *snip += -1 had the same problem which led to the index based rewrite (and TBH I'm not too fond of it either).

Pushed a new patch with a different approach. This one returns early on the nr_snips == 0 case making end - 1 computation well defined. And checks against != stop before the increment/decrement to avoid going outside the array.

This seems easier to understand than the index approach, let me know what you think.

EDIT: also renamed start & end to oldest & newest. The older name of start got a bit confusing with the introduction of stop pointer.

[cdown]

Thanks!