rustyrussell / ccan

The C Code Archive Network
http://ccodearchive.net/
1.11k stars 206 forks source link

darray.h: avoid UB when decrementing zero pointer #81

Open Hi-Angel opened 5 years ago

Hi-Angel commented 5 years ago

This PR is a backport of this one.

Sometimes the &(arr).item[(arr).size] is a zero pointer. In these cases decrementing this pointer aka i results in something like 0xfffffff8. This is UB, and UB sanitizer in particular reports it as

../iscsi/tcmu-runner/libtcmu.c:563:2: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffff8

In these cases size is zero as well, so fix this by simply not running the cycle when the size is zero.

Signed-off-by: Konstantin Kharlamov Hi-Angel@yandex.ru

dgibson commented 5 years ago

Ugh, this is a bit of a problem. The bug is real, and the patch does address it, but it's not actualy macro safe. If you had this code:

if (something)
    darray_foreach_reverse(...)
        ...
else
     something_else()

The else would get attached to the if in the macro which is almost certainly not what you wanted.

The usual trick of a do {} while wrapper obviously won't work here for a foreach() style macro. I think we have to try to fold the external condition into the loop condition instead.

Hi-Angel commented 5 years ago

Good note. I'm thinking something like this can do it:

#define darray_foreach_reverse(ptr, arr)           \
    for (size_t i = 0; i < (arr).size; (ptr) = &(arr).item[(arr).size - i], i++)

except this code doesn't do assignment to ptr on the first cycle. I think there was some hack to do assignment in part of for-loop where comparison happens, i.e. for(…; HERE;…), will try to figure it out a bit later (suggestions are welcome though :)).

Hi-Angel commented 5 years ago

Okay, done. Turns out, assignment in C returns the assigned value, so I used that trick to make it work.

Pushed, please see if you're okay with it.

Hi-Angel commented 5 years ago

Small update: I figured the assignment is a waste when comparison fails, so I swapped left and right parts of the …&&… expression in the loop.

Hi-Angel commented 8 months ago

ping