Message ID | 20200823095039.18851-3-andreas.rheinhardt@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/3] avfilter/graphparser: Fix leaks when parsing inputs fails | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
Andreas Rheinhardt (12020-08-23): > The result of the last check in code like > > if (p) > foo > else > p = av_mallocz(sizeof(*p)); > > if (p->ptr) > bar > else > bar2 > > is known in advance if the else branch of the first check was taken > because av_mallocz() returns zeroed buffers. Therefore the above snippet > can be simplified by moving the check and bar into the foo block. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavfilter/graphparser.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c > index a52916a146..f55737f8c7 100644 > --- a/libavfilter/graphparser.c > +++ b/libavfilter/graphparser.c > @@ -266,20 +266,20 @@ static int link_filter_inouts(AVFilterContext *filt_ctx, > if (p) { > *curr_inputs = (*curr_inputs)->next; > p->next = NULL; > + if (p->filter_ctx) { > + ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx); > + av_freep(&p->name); > + av_freep(&p); > + if (ret < 0) > + return ret; > + continue; > + } > } else if (!(p = av_mallocz(sizeof(*p)))) > return AVERROR(ENOMEM); > > - if (p->filter_ctx) { > - ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx); > - av_freep(&p->name); > - av_freep(&p); > - if (ret < 0) > - return ret; > - } else { > - p->filter_ctx = filt_ctx; > - p->pad_idx = pad; > - append_inout(open_inputs, &p); > - } > + p->filter_ctx = filt_ctx; > + p->pad_idx = pad; > + append_inout(open_inputs, &p); > } > > if (*curr_inputs) { I think I like the current version better. I had to read three times before I noticed the "continue" that guarantees the last three lines will not overwrite an existing filter. That means somebody could add a line there, expecting it to be valid for both cases. There is probably more simplification to be gained by reworking the whole logic into a more modular structure, but that is a task for later. Regards,
diff --git a/libavfilter/graphparser.c b/libavfilter/graphparser.c index a52916a146..f55737f8c7 100644 --- a/libavfilter/graphparser.c +++ b/libavfilter/graphparser.c @@ -266,20 +266,20 @@ static int link_filter_inouts(AVFilterContext *filt_ctx, if (p) { *curr_inputs = (*curr_inputs)->next; p->next = NULL; + if (p->filter_ctx) { + ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx); + av_freep(&p->name); + av_freep(&p); + if (ret < 0) + return ret; + continue; + } } else if (!(p = av_mallocz(sizeof(*p)))) return AVERROR(ENOMEM); - if (p->filter_ctx) { - ret = link_filter(p->filter_ctx, p->pad_idx, filt_ctx, pad, log_ctx); - av_freep(&p->name); - av_freep(&p); - if (ret < 0) - return ret; - } else { - p->filter_ctx = filt_ctx; - p->pad_idx = pad; - append_inout(open_inputs, &p); - } + p->filter_ctx = filt_ctx; + p->pad_idx = pad; + append_inout(open_inputs, &p); } if (*curr_inputs) {
The result of the last check in code like if (p) foo else p = av_mallocz(sizeof(*p)); if (p->ptr) bar else bar2 is known in advance if the else branch of the first check was taken because av_mallocz() returns zeroed buffers. Therefore the above snippet can be simplified by moving the check and bar into the foo block. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavfilter/graphparser.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)