Skip to content

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

Open
Hi-Angel wants to merge 1 commit into
rustyrussell:masterfrom
Hi-Angel:my-patch-backport
Open

darray.h: avoid UB when decrementing zero pointer#81
Hi-Angel wants to merge 1 commit into
rustyrussell:masterfrom
Hi-Angel:my-patch-backport

Conversation

@Hi-Angel

Copy link
Copy Markdown

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

dgibson commented Jul 28, 2019

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Author

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 Hi-Angel force-pushed the my-patch-backport branch from 45358e4 to 1e2d610 Compare July 28, 2019 12:01
@Hi-Angel

Copy link
Copy Markdown
Author

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.

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>
@Hi-Angel Hi-Angel force-pushed the my-patch-backport branch from 1e2d610 to f463655 Compare July 28, 2019 12:09
@Hi-Angel

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants