| Message ID | 20191129051600.29619-1-lq@chinaffmpeg.org |
|---|---|
| State | Accepted |
| Commit | b26225a3c7109400f2870daf94c2c731e29976f8 |
| Headers | show |
On Fri, Nov 29, 2019 at 1:16 PM Steven Liu <lq@chinaffmpeg.org> wrote: > > Signed-off-by: Steven Liu <lq@chinaffmpeg.org> > --- > libavformat/dashenc.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c > index a462876c13..8c28fb6b6e 100644 > --- a/libavformat/dashenc.c > +++ b/libavformat/dashenc.c > @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) > avio_close(os->ctx->pb); > } > ff_format_io_close(s, &os->out); > - if (os->ctx) > - avformat_free_context(os->ctx); > + avformat_free_context(os->ctx); > for (j = 0; j < os->nb_segments; j++) > av_free(os->segments[j]); > av_free(os->segments); > -- > 2.15.1 > Patchset LGTM
On 11/29/19 10:45 AM, Steven Liu wrote: > Signed-off-by: Steven Liu <lq@chinaffmpeg.org> > --- > libavformat/dashenc.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c > index a462876c13..8c28fb6b6e 100644 > --- a/libavformat/dashenc.c > +++ b/libavformat/dashenc.c > @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) > avio_close(os->ctx->pb); > } > ff_format_io_close(s, &os->out); > - if (os->ctx) > - avformat_free_context(os->ctx); > + avformat_free_context(os->ctx); This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. Unless this is absolutely required for some reason, this 'if' condition should not be removed. > for (j = 0; j < os->nb_segments; j++) > av_free(os->segments[j]); > av_free(os->segments);
> 在 2019年11月29日，13:35，Jeyapal, Karthick <kjeyapal@akamai.com> 写道： > > > On 11/29/19 10:45 AM, Steven Liu wrote: >> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >> --- >> libavformat/dashenc.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >> index a462876c13..8c28fb6b6e 100644 >> --- a/libavformat/dashenc.c >> +++ b/libavformat/dashenc.c >> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >> avio_close(os->ctx->pb); >> } >> ff_format_io_close(s, &os->out); >> - if (os->ctx) >> - avformat_free_context(os->ctx); >> + avformat_free_context(os->ctx); > This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. > Unless this is absolutely required for some reason, this 'if' condition should not be removed. 4433 void avformat_free_context(AVFormatContext *s) 4434 { 4435 int i; 4436 4437 if (!s) 4438 return; This check is in the function of avformat_free_context. >> for (j = 0; j < os->nb_segments; j++) >> av_free(os->segments[j]); >> av_free(os->segments); > Thanks Steven
On 11/29/19 11:08 AM, Steven Liu wrote: > > >> 在 2019年11月29日，13:35，Jeyapal, Karthick <kjeyapal@akamai.com> 写道： >> >> >> On 11/29/19 10:45 AM, Steven Liu wrote: >>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>> --- >>> libavformat/dashenc.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>> index a462876c13..8c28fb6b6e 100644 >>> --- a/libavformat/dashenc.c >>> +++ b/libavformat/dashenc.c >>> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >>> avio_close(os->ctx->pb); >>> } >>> ff_format_io_close(s, &os->out); >>> - if (os->ctx) >>> - avformat_free_context(os->ctx); >>> + avformat_free_context(os->ctx); >> This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. >> Unless this is absolutely required for some reason, this 'if' condition should not be removed. > > > 4433 void avformat_free_context(AVFormatContext *s) > 4434 { > 4435 int i; > 4436 > 4437 if (!s) > 4438 return; > > This check is in the function of avformat_free_context. Great! Then it is fine. You can remove it. > >>> for (j = 0; j < os->nb_segments; j++) >>> av_free(os->segments[j]); >>> av_free(os->segments); >> > > Thanks > Steven > > > > >
Jeyapal, Karthick: > > On 11/29/19 10:45 AM, Steven Liu wrote: >> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >> --- >> libavformat/dashenc.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >> index a462876c13..8c28fb6b6e 100644 >> --- a/libavformat/dashenc.c >> +++ b/libavformat/dashenc.c >> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >> avio_close(os->ctx->pb); >> } >> ff_format_io_close(s, &os->out); >> - if (os->ctx) >> - avformat_free_context(os->ctx); >> + avformat_free_context(os->ctx); > This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. > Unless this is absolutely required for some reason, this 'if' condition should not be removed. This 'if' doesn't help against double-frees at all: avformat_free_context() itself checks against its argument being NULL and returns immediately if it is. (But double frees are still possible given that avformat_free_context can't reset the pointer to NULL due to its signature. Resetting is something the callers have to do for themselves.) - Andreas
> 在 2019年11月29日，13:39，Jeyapal, Karthick <kjeyapal@akamai.com> 写道： > > > On 11/29/19 11:08 AM, Steven Liu wrote: >> >> >>> 在 2019年11月29日，13:35，Jeyapal, Karthick <kjeyapal@akamai.com> 写道： >>> >>> >>> On 11/29/19 10:45 AM, Steven Liu wrote: >>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>>> --- >>>> libavformat/dashenc.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>>> index a462876c13..8c28fb6b6e 100644 >>>> --- a/libavformat/dashenc.c >>>> +++ b/libavformat/dashenc.c >>>> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >>>> avio_close(os->ctx->pb); >>>> } >>>> ff_format_io_close(s, &os->out); >>>> - if (os->ctx) >>>> - avformat_free_context(os->ctx); >>>> + avformat_free_context(os->ctx); >>> This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. >>> Unless this is absolutely required for some reason, this 'if' condition should not be removed. >> >> >> 4433 void avformat_free_context(AVFormatContext *s) >> 4434 { >> 4435 int i; >> 4436 >> 4437 if (!s) >> 4438 return; >> >> This check is in the function of avformat_free_context. > Great! Then it is fine. You can remove it. Thanks Jeyapal :) >> >>>> for (j = 0; j < os->nb_segments; j++) >>>> av_free(os->segments[j]); >>>> av_free(os->segments); >>> >> >> Thanks >> Steven Thanks Steven
> 在 2019年11月29日，13:40，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Jeyapal, Karthick: >> >> On 11/29/19 10:45 AM, Steven Liu wrote: >>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>> --- >>> libavformat/dashenc.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>> index a462876c13..8c28fb6b6e 100644 >>> --- a/libavformat/dashenc.c >>> +++ b/libavformat/dashenc.c >>> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >>> avio_close(os->ctx->pb); >>> } >>> ff_format_io_close(s, &os->out); >>> - if (os->ctx) >>> - avformat_free_context(os->ctx); >>> + avformat_free_context(os->ctx); >> This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. >> Unless this is absolutely required for some reason, this 'if' condition should not be removed. > > This 'if' doesn't help against double-frees at all: > avformat_free_context() itself checks against its argument being NULL > and returns immediately if it is. (But double frees are still possible > given that avformat_free_context can't reset the pointer to NULL due > to its signature. Resetting is something the callers have to do for > themselves.) What about add reset pointer to NULL into the avformat_free_context? > > - 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". Thanks Steven
Steven Liu: > > >> 在 2019年11月29日，13:40，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >> >> Jeyapal, Karthick: >>> >>> On 11/29/19 10:45 AM, Steven Liu wrote: >>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>>> --- >>>> libavformat/dashenc.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>>> index a462876c13..8c28fb6b6e 100644 >>>> --- a/libavformat/dashenc.c >>>> +++ b/libavformat/dashenc.c >>>> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >>>> avio_close(os->ctx->pb); >>>> } >>>> ff_format_io_close(s, &os->out); >>>> - if (os->ctx) >>>> - avformat_free_context(os->ctx); >>>> + avformat_free_context(os->ctx); >>> This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. >>> Unless this is absolutely required for some reason, this 'if' condition should not be removed. >> >> This 'if' doesn't help against double-frees at all: >> avformat_free_context() itself checks against its argument being NULL >> and returns immediately if it is. (But double frees are still possible >> given that avformat_free_context can't reset the pointer to NULL due >> to its signature. Resetting is something the callers have to do for >> themselves.) > What about add reset pointer to NULL into the avformat_free_context? This is pointless, because it will just reset the functions copy of the pointer to NULL, not the callers pointer to the AVFormatContext. In order to reset the latter, the parameter would have to be of type AVFormatContext ** (and the call here would be avformat_free_context(&os->ctx)). - Andreas
> 在 2019年11月29日，13:57，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： > > Steven Liu: >> >> >>> 在 2019年11月29日，13:40，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >>> >>> Jeyapal, Karthick: >>>> >>>> On 11/29/19 10:45 AM, Steven Liu wrote: >>>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>>>> --- >>>>> libavformat/dashenc.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>>>> index a462876c13..8c28fb6b6e 100644 >>>>> --- a/libavformat/dashenc.c >>>>> +++ b/libavformat/dashenc.c >>>>> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >>>>> avio_close(os->ctx->pb); >>>>> } >>>>> ff_format_io_close(s, &os->out); >>>>> - if (os->ctx) >>>>> - avformat_free_context(os->ctx); >>>>> + avformat_free_context(os->ctx); >>>> This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. >>>> Unless this is absolutely required for some reason, this 'if' condition should not be removed. >>> >>> This 'if' doesn't help against double-frees at all: >>> avformat_free_context() itself checks against its argument being NULL >>> and returns immediately if it is. (But double frees are still possible >>> given that avformat_free_context can't reset the pointer to NULL due >>> to its signature. Resetting is something the callers have to do for >>> themselves.) >> What about add reset pointer to NULL into the avformat_free_context? > > This is pointless, because it will just reset the functions copy of > the pointer to NULL, not the callers pointer to the AVFormatContext. > In order to reset the latter, the parameter would have to be of type > AVFormatContext ** (and the call here would be > avformat_free_context(&os->ctx)). AVFormatContext **? Isn’t that too complex for user of the API caller? > > - 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". Thanks Steven
Steven Liu: > > >> 在 2019年11月29日，13:57，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >> >> Steven Liu: >>> >>> >>>> 在 2019年11月29日，13:40，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道： >>>> >>>> Jeyapal, Karthick: >>>>> >>>>> On 11/29/19 10:45 AM, Steven Liu wrote: >>>>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org> >>>>>> --- >>>>>> libavformat/dashenc.c | 3 +-- >>>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c >>>>>> index a462876c13..8c28fb6b6e 100644 >>>>>> --- a/libavformat/dashenc.c >>>>>> +++ b/libavformat/dashenc.c >>>>>> @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) >>>>>> avio_close(os->ctx->pb); >>>>>> } >>>>>> ff_format_io_close(s, &os->out); >>>>>> - if (os->ctx) >>>>>> - avformat_free_context(os->ctx); >>>>>> + avformat_free_context(os->ctx); >>>>> This 'if' condition is a safety net against double frees caused due to any corner case and/or wrong usage. >>>>> Unless this is absolutely required for some reason, this 'if' condition should not be removed. >>>> >>>> This 'if' doesn't help against double-frees at all: >>>> avformat_free_context() itself checks against its argument being NULL >>>> and returns immediately if it is. (But double frees are still possible >>>> given that avformat_free_context can't reset the pointer to NULL due >>>> to its signature. Resetting is something the callers have to do for >>>> themselves.) >>> What about add reset pointer to NULL into the avformat_free_context? >> >> This is pointless, because it will just reset the functions copy of >> the pointer to NULL, not the callers pointer to the AVFormatContext. >> In order to reset the latter, the parameter would have to be of type >> AVFormatContext ** (and the call here would be >> avformat_free_context(&os->ctx)). > AVFormatContext **? Isn’t that too complex for user of the API caller? I don't see anything complex in such a parameter. And this is actually a quite common scenario (just think of av_freep() or avio_context_free() which both need **-arguments to do their job). - Andreas
diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c index a462876c13..8c28fb6b6e 100644 --- a/libavformat/dashenc.c +++ b/libavformat/dashenc.c @@ -588,8 +588,7 @@ static void dash_free(AVFormatContext *s) avio_close(os->ctx->pb); } ff_format_io_close(s, &os->out); - if (os->ctx) - avformat_free_context(os->ctx); + avformat_free_context(os->ctx); for (j = 0; j < os->nb_segments; j++) av_free(os->segments[j]); av_free(os->segments);
Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/dashenc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)