| Message ID | 20210214212854.282253-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 9c0b3eddf4262f9dcea479091f1307444e614e88 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/utils: Fix undefined NULL + 0 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
probably fine
On 2/14/2021 6:28 PM, Andreas Rheinhardt wrote: > This is undefined behaviour in C, so use data = len ? data + len : data > instead if data += len. GCC optimizes the branch away in this case; > Clang unfortunately doesn't. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Checking for len != 0 instead of > 0 allows the compiler to optimize the > branch away; maybe future versions of Clang (I used 11) will do so, too. > > libavformat/utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 3e955b85bc..cea6d4ca92 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -1426,7 +1426,7 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, > pkt->pts = pkt->dts = AV_NOPTS_VALUE; > pkt->pos = -1; > /* increment read pointer */ > - data += len; > + data = len ? data + len : data; Isn't adding any offset to a NULL pointer UB? That was afaik the entire point behind the change in c40d36076a. > size -= len; > > got_output = !!out_pkt.size; >
James Almer: > On 2/14/2021 6:28 PM, Andreas Rheinhardt wrote: >> This is undefined behaviour in C, so use data = len ? data + len : data >> instead if data += len. GCC optimizes the branch away in this case; >> Clang unfortunately doesn't. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> Checking for len != 0 instead of > 0 allows the compiler to optimize the >> branch away; maybe future versions of Clang (I used 11) will do so, too. >> >> libavformat/utils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index 3e955b85bc..cea6d4ca92 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -1426,7 +1426,7 @@ static int parse_packet(AVFormatContext *s, >> AVPacket *pkt, >> pkt->pts = pkt->dts = AV_NOPTS_VALUE; >> pkt->pos = -1; >> /* increment read pointer */ >> - data += len; >> + data = len ? data + len : data; > > Isn't adding any offset to a NULL pointer UB? That was afaik the entire > point behind the change in c40d36076a. > Yes, it is. And if it happened here, it would indicate a real bug (i.e. a real logic error) that would need to be fixed; but as long as it doesn't happen, we should apply cheap solutions that the compiler can optimize away (you probably notice that I regard NULL + 0 as not a real bug, but rather as an annoyance of the C standard that disallows this (even the sanitizer authors seem to agree with this point (that's why there is an error message specifically for NULL + 0); it is allowed in C++). - Andreas
diff --git a/libavformat/utils.c b/libavformat/utils.c index 3e955b85bc..cea6d4ca92 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -1426,7 +1426,7 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, pkt->pts = pkt->dts = AV_NOPTS_VALUE; pkt->pos = -1; /* increment read pointer */ - data += len; + data = len ? data + len : data; size -= len; got_output = !!out_pkt.size;
This is undefined behaviour in C, so use data = len ? data + len : data instead if data += len. GCC optimizes the branch away in this case; Clang unfortunately doesn't. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Checking for len != 0 instead of > 0 allows the compiler to optimize the branch away; maybe future versions of Clang (I used 11) will do so, too. libavformat/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)