| Message ID | 1514824117-15587-2-git-send-email-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 01, 2018 at 11:28:36AM -0500, Derek Buitenhuis wrote: > This fixes a segfault caused by passing NULL to ff_filter_frame > when an error occurs. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavfilter/vf_paletteuse.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c > index 1980907..ede2e2e 100644 > --- a/libavfilter/vf_paletteuse.c > +++ b/libavfilter/vf_paletteuse.c > @@ -894,9 +894,9 @@ static void set_processing_window(enum diff_mode diff_mode, > *hp = height; > } > > -static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in) > +static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf) > { > - int x, y, w, h; > + int x, y, w, h, ret; > AVFilterContext *ctx = inlink->dst; > PaletteUseContext *s = ctx->priv; > AVFilterLink *outlink = inlink->dst->outputs[0]; > @@ -904,7 +904,8 @@ static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in) > AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h); > if (!out) { > av_frame_free(&in); > - return NULL; > + *outf = NULL; > + return AVERROR(EINVAL); > } > av_frame_copy_props(out, in); > > @@ -918,21 +919,25 @@ static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in) > av_frame_make_writable(s->last_in) < 0) { > av_frame_free(&in); > av_frame_free(&out); > - return NULL; > + *outf = NULL; > + return AVERROR(EINVAL); > } > > ff_dlog(ctx, "%dx%d rect: (%d;%d) -> (%d,%d) [area:%dx%d]\n", > w, h, x, y, x+w, y+h, in->width, in->height); > > - if (s->set_frame(s, out, in, x, y, w, h) < 0) { > + ret = s->set_frame(s, out, in, x, y, w, h); > + if (ret < 0) { > av_frame_free(&out); > - return NULL; > + *outf = NULL; > + return ret; > } > memcpy(out->data[1], s->palette, AVPALETTE_SIZE); > if (s->calc_mean_err) > debug_mean_error(s, in, out, inlink->frame_count_out); > av_frame_free(&in); > - return out; > + *outf = out; > + return 0; > } > > static int config_output(AVFilterLink *outlink) > @@ -1011,7 +1016,7 @@ static int load_apply_palette(FFFrameSync *fs) > AVFilterContext *ctx = fs->parent; > AVFilterLink *inlink = ctx->inputs[0]; > PaletteUseContext *s = ctx->priv; > - AVFrame *master, *second, *out; > + AVFrame *master, *second, *out = NULL; > int ret; > > // writable for error diffusal dithering > @@ -1025,7 +1030,9 @@ static int load_apply_palette(FFFrameSync *fs) > if (!s->palette_loaded) { > load_palette(s, second); > } > - out = apply_palette(inlink, master); > + ret = apply_palette(inlink, master, &out); > + if (ret < 0) > + goto error; > return ff_filter_frame(ctx->outputs[0], out); > not exactly sure why you haven't just checked if `out` wasn't NULL, but it should be fine that way too if you prefer it. I guess that's only to provide a finer grain error handling? It would make sense if ff_get_video_buffer was returning a meaningful error, but so far you're just assuming EINVAL when it could be ENOMEM, so I don't really get it.
On 1/2/2018 9:52 PM, Clément Bœsch wrote: > not exactly sure why you haven't just checked if `out` wasn't NULL, but it > should be fine that way too if you prefer it. > > I guess that's only to provide a finer grain error handling? It would make > sense if ff_get_video_buffer was returning a meaningful error, but so far > you're just assuming EINVAL when it could be ENOMEM, so I don't really get > it. s->set_frame can return ENOMEM, which is why I made it finer grained. I'm not really sure what to return for ff_get_video_buffer failure, since it wasn't designed with any error mechanism for some reason. - Derek
On Tue, Jan 02, 2018 at 10:02:25PM +0000, Derek Buitenhuis wrote: > On 1/2/2018 9:52 PM, Clément Bœsch wrote: > > not exactly sure why you haven't just checked if `out` wasn't NULL, but it > > should be fine that way too if you prefer it. > > > > I guess that's only to provide a finer grain error handling? It would make > > sense if ff_get_video_buffer was returning a meaningful error, but so far > > you're just assuming EINVAL when it could be ENOMEM, so I don't really get > > it. > > s->set_frame can return ENOMEM, which is why I made it finer grained. > > I'm not really sure what to return for ff_get_video_buffer failure, since > it wasn't designed with any error mechanism for some reason. > I don't think you'll be much off by always assuming ENOMEM here when getting a NULL out frame, I think that's the common idiom when a function supposed to return allocated stuff returns NULL. But that's not very important, feel free to push as is if you prefer that way.
On 1/2/2018 10:16 PM, Clément Bœsch wrote: > I don't think you'll be much off by always assuming ENOMEM here when > getting a NULL out frame, I think that's the common idiom when a function > supposed to return allocated stuff returns NULL. > > But that's not very important, feel free to push as is if you prefer that > way. I'll change it to ENOMEM before push, then. - Derek
diff --git a/libavfilter/vf_paletteuse.c b/libavfilter/vf_paletteuse.c index 1980907..ede2e2e 100644 --- a/libavfilter/vf_paletteuse.c +++ b/libavfilter/vf_paletteuse.c @@ -894,9 +894,9 @@ static void set_processing_window(enum diff_mode diff_mode, *hp = height; } -static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in) +static int apply_palette(AVFilterLink *inlink, AVFrame *in, AVFrame **outf) { - int x, y, w, h; + int x, y, w, h, ret; AVFilterContext *ctx = inlink->dst; PaletteUseContext *s = ctx->priv; AVFilterLink *outlink = inlink->dst->outputs[0]; @@ -904,7 +904,8 @@ static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in) AVFrame *out = ff_get_video_buffer(outlink, outlink->w, outlink->h); if (!out) { av_frame_free(&in); - return NULL; + *outf = NULL; + return AVERROR(EINVAL); } av_frame_copy_props(out, in); @@ -918,21 +919,25 @@ static AVFrame *apply_palette(AVFilterLink *inlink, AVFrame *in) av_frame_make_writable(s->last_in) < 0) { av_frame_free(&in); av_frame_free(&out); - return NULL; + *outf = NULL; + return AVERROR(EINVAL); } ff_dlog(ctx, "%dx%d rect: (%d;%d) -> (%d,%d) [area:%dx%d]\n", w, h, x, y, x+w, y+h, in->width, in->height); - if (s->set_frame(s, out, in, x, y, w, h) < 0) { + ret = s->set_frame(s, out, in, x, y, w, h); + if (ret < 0) { av_frame_free(&out); - return NULL; + *outf = NULL; + return ret; } memcpy(out->data[1], s->palette, AVPALETTE_SIZE); if (s->calc_mean_err) debug_mean_error(s, in, out, inlink->frame_count_out); av_frame_free(&in); - return out; + *outf = out; + return 0; } static int config_output(AVFilterLink *outlink) @@ -1011,7 +1016,7 @@ static int load_apply_palette(FFFrameSync *fs) AVFilterContext *ctx = fs->parent; AVFilterLink *inlink = ctx->inputs[0]; PaletteUseContext *s = ctx->priv; - AVFrame *master, *second, *out; + AVFrame *master, *second, *out = NULL; int ret; // writable for error diffusal dithering @@ -1025,7 +1030,9 @@ static int load_apply_palette(FFFrameSync *fs) if (!s->palette_loaded) { load_palette(s, second); } - out = apply_palette(inlink, master); + ret = apply_palette(inlink, master, &out); + if (ret < 0) + goto error; return ff_filter_frame(ctx->outputs[0], out); error:
This fixes a segfault caused by passing NULL to ff_filter_frame when an error occurs. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavfilter/vf_paletteuse.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)