| Message ID | 20210503040743.54346-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avutil/imgutils: don't add offsets to NULL pointers | 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 |
James Almer: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavutil/imgutils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c > index 53faad889a..aaee0dfb7a 100644 > --- a/libavutil/imgutils.c > +++ b/libavutil/imgutils.c > @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei > } > > data[0] = ptr; > - for (i = 1; i < 4 && sizes[i]; i++) > + for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++) > data[i] = data[i - 1] + sizes[i - 1]; > > return ret; > I see two ways to make this a NULL + offset: First, if ptr == NULL; and second if data[i - 1] + sizes[i - 1] no longer fits into the allocated buffer and happens to yield NULL (very unlikely, but possible) in which case data[i] + sizes[i] would be NULL + offset. In the second case, the first addition is already undefined behaviour against which we cannot guard at all: We don't know the size of the buffer. The only thing we can guard against is ptr being NULL; we can even error out in this scenario, but I don't know how disruptive that would be. Notice that in C the result of pointer + offset can never be NULL, so a compiler could optimize the check for data[i - 1] to just a check for ptr. - Andreas
On 5/4/2021 5:13 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavutil/imgutils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c >> index 53faad889a..aaee0dfb7a 100644 >> --- a/libavutil/imgutils.c >> +++ b/libavutil/imgutils.c >> @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei >> } >> >> data[0] = ptr; >> - for (i = 1; i < 4 && sizes[i]; i++) >> + for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++) >> data[i] = data[i - 1] + sizes[i - 1]; >> >> return ret; >> I see two ways to make this a NULL + offset: First, if ptr == NULL; and > second if data[i - 1] + sizes[i - 1] no longer fits into the allocated > buffer and happens to yield NULL (very unlikely, but possible) in which > case data[i] + sizes[i] would be NULL + offset. In the second case, the > first addition is already undefined behaviour against which we cannot > guard at all: We don't know the size of the buffer. The only thing we > can guard against is ptr being NULL; we can even error out in this > scenario, but I don't know how disruptive that would be. That'd be an undesirable breakage, yes. Aside from filling data[], the function also returns the size of the buffer that should be allocated, so that functionality should remain even when ptr == NULL. > Notice that in C the result of pointer + offset can never be NULL, so a > compiler could optimize the check for data[i - 1] to just a check for ptr. If you say there's no warranty that an scenario where data[i-1] + size[i-1] == NULL will break the for loop in the next iteration, and no way to guard against it at all, then we can just return right before attempting to set data[] when ptr == NULL, and at least simplify that scenario. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 5/4/2021 5:50 PM, James Almer wrote: > On 5/4/2021 5:13 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavutil/imgutils.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c >>> index 53faad889a..aaee0dfb7a 100644 >>> --- a/libavutil/imgutils.c >>> +++ b/libavutil/imgutils.c >>> @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum >>> AVPixelFormat pix_fmt, int hei >>> } >>> data[0] = ptr; >>> - for (i = 1; i < 4 && sizes[i]; i++) >>> + for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++) >>> data[i] = data[i - 1] + sizes[i - 1]; >>> return ret; >>> I see two ways to make this a NULL + offset: First, if ptr == NULL; and >> second if data[i - 1] + sizes[i - 1] no longer fits into the allocated >> buffer and happens to yield NULL (very unlikely, but possible) in which >> case data[i] + sizes[i] would be NULL + offset. In the second case, the >> first addition is already undefined behaviour against which we cannot >> guard at all: We don't know the size of the buffer. The only thing we >> can guard against is ptr being NULL; we can even error out in this >> scenario, but I don't know how disruptive that would be. > > That'd be an undesirable breakage, yes. Aside from filling data[], the > function also returns the size of the buffer that should be allocated, > so that functionality should remain even when ptr == NULL. > >> Notice that in C the result of pointer + offset can never be NULL, so a >> compiler could optimize the check for data[i - 1] to just a check for >> ptr. > > If you say there's no warranty that an scenario where data[i-1] + > size[i-1] == NULL will break the for loop in the next iteration, and no > way to guard against it at all, then we can just return right before > attempting to set data[] when ptr == NULL, and at least simplify that > scenario. Made that change and pushed.
diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c index 53faad889a..aaee0dfb7a 100644 --- a/libavutil/imgutils.c +++ b/libavutil/imgutils.c @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei } data[0] = ptr; - for (i = 1; i < 4 && sizes[i]; i++) + for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++) data[i] = data[i - 1] + sizes[i - 1]; return ret;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavutil/imgutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)