| Message ID | 20190516050217.26004-1-ruiling.song@intel.com |
|---|---|
| State | New |
| Headers | show |
Ruiling Song (12019-05-16): > ctx is a pointer to pointer here. > > Signed-off-by: Ruiling Song <ruiling.song@intel.com> > --- > libavutil/tx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/tx.c b/libavutil/tx.c > index 934ef27c81..1690604040 100644 > --- a/libavutil/tx.c > +++ b/libavutil/tx.c > @@ -697,7 +697,7 @@ static int gen_mdct_exptab(AVTXContext *s, int len4, double scale) > > av_cold void av_tx_uninit(AVTXContext **ctx) > { > - if (!ctx) > + if (!ctx || !(*ctx)) That would protect somebody stupid enough to call av_tx_uninit(NULL) instead of av_tx_uninit(&var). A hard crass is completely warranted in this case. An assert would be acceptable. > return; > > av_free((*ctx)->pfatab); Regards,
May 16, 2019, 6:02 AM by ruiling.song@intel.com: > ctx is a pointer to pointer here. > > Signed-off-by: Ruiling Song <> ruiling.song@intel.com <mailto:ruiling.song@intel.com>> > > --- > libavutil/tx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/tx.c b/libavutil/tx.c > index 934ef27c81..1690604040 100644 > --- a/libavutil/tx.c > +++ b/libavutil/tx.c > @@ -697,7 +697,7 @@ static int gen_mdct_exptab(AVTXContext *s, int len4, double scale) > > av_cold void av_tx_uninit(AVTXContext **ctx) > { > - if (!ctx) > + if (!ctx || !(*ctx)) > return; > > av_free((*ctx)->pfatab); > LGTM
Lynne (12019-05-16):
> LGTM
As I said, not to me. Please take all comments into account.
Regards,
>Ruiling Song (12019-05-16): >> ctx is a pointer to pointer here. >> >> Signed-off-by: Ruiling Song <ruiling.song@intel.com> >> --- >> libavutil/tx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavutil/tx.c b/libavutil/tx.c >> index 934ef27c81..1690604040 100644 >> --- a/libavutil/tx.c >> +++ b/libavutil/tx.c >> @@ -697,7 +697,7 @@ static int gen_mdct_exptab(AVTXContext *s, int len4, double scale) >> >> av_cold void av_tx_uninit(AVTXContext **ctx) >> { > >> - if (!ctx) >> + if (!ctx || !(*ctx)) > >That would protect somebody stupid enough to call av_tx_uninit(NULL) >instead of av_tx_uninit(&var). A hard crass is completely warranted in >this case. An assert would be acceptable. Actually that is what the original code does. What you appear to want is if (!*ctx) which protects against multi-free and is useful in that it can be called unconditionally in cleanup code (assuming initial null assignments) and crashes in what you describe as the "stupid" case. >> return; >> >> av_free((*ctx)->pfatab); > >Regards, Regards John Cox
May 16, 2019, 11:35 AM by george@nsup.org: > Lynne (12019-05-16): > >> LGTM >> > > As I said, not to me. Please take all comments into account. > Its fine by me. Adding an AVClass is unwarranted and absolutely no standalone API we've ever used like av_fft or av_has has a class.
On 5/16/2019 5:03 AM, Nicolas George wrote: > Ruiling Song (12019-05-16): >> ctx is a pointer to pointer here. >> >> Signed-off-by: Ruiling Song <ruiling.song@intel.com> >> --- >> libavutil/tx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavutil/tx.c b/libavutil/tx.c >> index 934ef27c81..1690604040 100644 >> --- a/libavutil/tx.c >> +++ b/libavutil/tx.c >> @@ -697,7 +697,7 @@ static int gen_mdct_exptab(AVTXContext *s, int len4, double scale) >> >> av_cold void av_tx_uninit(AVTXContext **ctx) >> { > >> - if (!ctx) >> + if (!ctx || !(*ctx)) > > That would protect somebody stupid enough to call av_tx_uninit(NULL) > instead of av_tx_uninit(&var). A hard crass is completely warranted in > this case. An assert would be acceptable. An assert is meant to detect developer errors, not user errors. Crashing the user's whole application because they misused the API is not really acceptable. I can't find examples of such functions using asserts this way, but there are several uninit/free/unref functions behave like the above patch. See av_buffer_unref(), av_packet_free(), av_bsf_free(). Other functions instead just don't even consider the passed argument could be NULL at all, like avcodec_free_context() and swr_free(), which while not 100% safe, is a pretty realistic expectation. I'd say either apply this patch as is, or apply the original one sent last night. In both cases it will be following an existing precedent in the codebase. > >> return; >> >> av_free((*ctx)->pfatab); > > Regards, > > > _______________________________________________ > 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". >
John Cox (12019-05-16): > >> - if (!ctx) > >> + if (!ctx || !(*ctx)) > >That would protect somebody stupid enough to call av_tx_uninit(NULL) > >instead of av_tx_uninit(&var). A hard crass is completely warranted in > >this case. An assert would be acceptable. > Actually that is what the original code does. What you appear to want > is > > if (!*ctx) Indeed. It is for example what avfilter_link_free(), av_bsf_list_free(), etc., do. It correspond to a useful programming pattern. The current code is clearly a mistake. Regards,
James Almer (12019-05-16): > An assert is meant to detect developer errors, not user errors. Crashing > the user's whole application because they misused the API is not really > acceptable. > > I can't find examples of such functions using asserts this way, but > there are several uninit/free/unref functions behave like the above > patch. See av_buffer_unref(), av_packet_free(), av_bsf_free(). > Other functions instead just don't even consider the passed argument > could be NULL at all, like avcodec_free_context() and swr_free(), which > while not 100% safe, is a pretty realistic expectation. > > I'd say either apply this patch as is, or apply the original one sent > last night. In both cases it will be following an existing precedent in > the codebase. Re-read the code more carefully, and look what the existing predecent does. You made the same mistake as me. Regards,
On 5/16/2019 11:31 AM, Nicolas George wrote: > James Almer (12019-05-16): >> An assert is meant to detect developer errors, not user errors. Crashing >> the user's whole application because they misused the API is not really >> acceptable. >> >> I can't find examples of such functions using asserts this way, but >> there are several uninit/free/unref functions behave like the above >> patch. See av_buffer_unref(), av_packet_free(), av_bsf_free(). >> Other functions instead just don't even consider the passed argument >> could be NULL at all, like avcodec_free_context() and swr_free(), which >> while not 100% safe, is a pretty realistic expectation. >> >> I'd say either apply this patch as is, or apply the original one sent >> last night. In both cases it will be following an existing precedent in >> the codebase. > > Re-read the code more carefully, and look what the existing predecent > does. You made the same mistake as me. > > Regards, There are two precedents in the codebase, one checking for both the passed argument and then the struct pointer pointed by it (av_bsf_free and av_buffer_unref as i mentioned above), and one checking only the struct pointer (av_bsf_list_free as you mentioned in another email). The current code is wrong as already established. This patch applying (!ctx || !*ctx), and the one sent last night applying (!*ctx) are both correct, each of them implementing one of the two aforementioned precedents. The former just takes extra precautions against user error.
James Almer (12019-05-16): > There are two precedents in the codebase, one checking for both the > passed argument and then the struct pointer pointed by it (av_bsf_free > and av_buffer_unref as i mentioned above), and one checking only the > struct pointer (av_bsf_list_free as you mentioned in another email). There are a few other precedent. I think the majority dereference the pointer and only accept indirect NULL. This is not surprising: freeing something unconditionally is very useful when uniniting after a failure case or different branches of programming. On the other hand, freeing nothing is not a useful pattern. > The current code is wrong as already established. This patch applying > (!ctx || !*ctx), and the one sent last night applying (!*ctx) are both > correct, each of them implementing one of the two aforementioned Ok, I had missed there was another patch. > precedents. The former just takes extra precautions against user error. Not user: application programmer. This is a very important difference: users are supposed clueless, application programmers are supposed competent, even when they are not. That kind of mistake in using the API is probably a simple but grave mistake in the code, like writing av_foo_free(foo) instead of av_foo_free(&foo) (and ignoring the resulting warning). If ignored, it will lead to invalid frees or memory leaks in the application. This is not a service for the programmer: better have the program crash hard (either with an assert or a NULL deref) immediately, so they can fix it. Regards,
On 5/16/2019 3:11 PM, Nicolas George wrote: > James Almer (12019-05-16): >> There are two precedents in the codebase, one checking for both the >> passed argument and then the struct pointer pointed by it (av_bsf_free >> and av_buffer_unref as i mentioned above), and one checking only the >> struct pointer (av_bsf_list_free as you mentioned in another email). > > There are a few other precedent. I think the majority dereference the > pointer and only accept indirect NULL. This is not surprising: freeing > something unconditionally is very useful when uniniting after a failure > case or different branches of programming. On the other hand, freeing > nothing is not a useful pattern. > >> The current code is wrong as already established. This patch applying >> (!ctx || !*ctx), and the one sent last night applying (!*ctx) are both >> correct, each of them implementing one of the two aforementioned > > Ok, I had missed there was another patch. > >> precedents. The former just takes extra precautions against user error. > > Not user: application programmer. This is a very important difference: > users are supposed clueless, application programmers are supposed > competent, even when they are not. I meant it as library user, which is the application programmer and not the consumer/end user, but yes, you're right they should know better. > > That kind of mistake in using the API is probably a simple but grave > mistake in the code, like writing av_foo_free(foo) instead of > av_foo_free(&foo) (and ignoring the resulting warning). If ignored, it > will lead to invalid frees or memory leaks in the application. This is > not a service for the programmer: better have the program crash hard > (either with an assert or a NULL deref) immediately, so they can fix it. > > Regards, In that case I'm fine with using the single (!*ctx) check from the first patch.
May 16, 2019, 7:22 PM by jamrial@gmail.com: > On 5/16/2019 3:11 PM, Nicolas George wrote: > >> James Almer (12019-05-16): >> >>> There are two precedents in the codebase, one checking for both the >>> passed argument and then the struct pointer pointed by it (av_bsf_free >>> and av_buffer_unref as i mentioned above), and one checking only the >>> struct pointer (av_bsf_list_free as you mentioned in another email). >>> >> >> There are a few other precedent. I think the majority dereference the >> pointer and only accept indirect NULL. This is not surprising: freeing >> something unconditionally is very useful when uniniting after a failure >> case or different branches of programming. On the other hand, freeing >> nothing is not a useful pattern. >> >>> The current code is wrong as already established. This patch applying >>> (!ctx || !*ctx), and the one sent last night applying (!*ctx) are both >>> correct, each of them implementing one of the two aforementioned >>> >> >> Ok, I had missed there was another patch. >> >>> precedents. The former just takes extra precautions against user error. >>> >> >> Not user: application programmer. This is a very important difference: >> users are supposed clueless, application programmers are supposed >> competent, even when they are not. >> > > I meant it as library user, which is the application programmer and not > the consumer/end user, but yes, you're right they should know better. > >> >> That kind of mistake in using the API is probably a simple but grave >> mistake in the code, like writing av_foo_free(foo) instead of >> av_foo_free(&foo) (and ignoring the resulting warning). If ignored, it >> will lead to invalid frees or memory leaks in the application. This is >> not a service for the programmer: better have the program crash hard >> (either with an assert or a NULL deref) immediately, so they can fix it. >> >> Regards, >> > In that case I'm fine with using the single (!*ctx) check from the first > patch. > I'm not, I still want the 2 checks.
Lynne (12019-05-16):
> I'm not, I still want the 2 checks.
Arguments please. As I explained, the first check is harmful to
applications because it hides bug.
Regards,
May 16, 2019, 8:43 PM by george@nsup.org: > Lynne (12019-05-16): > >> I'm not, I still want the 2 checks. >> > > Arguments please. As I explained, the first check is harmful to > applications because it hides bug. > Nevermind, its not really possible to give av_tx_uninit() a NULL double pointer so whatever. Can the author just push a patch already?
On 5/16/2019 6:06 PM, Lynne wrote: > May 16, 2019, 8:43 PM by george@nsup.org: > >> Lynne (12019-05-16): >> >>> I'm not, I still want the 2 checks. >>> >> >> Arguments please. As I explained, the first check is harmful to >> applications because it hides bug. >> > > Nevermind, its not really possible to give av_tx_uninit() a NULL double pointer so > whatever. Can the author just push a patch already? I did it just now, but as the author and maintainer of the code in question you're the most adequate person to review and push patches for it.
May 16, 2019, 10:29 PM by jamrial@gmail.com: > On 5/16/2019 6:06 PM, Lynne wrote: > >> May 16, 2019, 8:43 PM by >> george@nsup.org <mailto:george@nsup.org>>> : >> >>> Lynne (12019-05-16): >>> >>>> I'm not, I still want the 2 checks. >>>> >>> >>> Arguments please. As I explained, the first check is harmful to >>> applications because it hides bug. >>> >> >> Nevermind, its not really possible to give av_tx_uninit() a NULL double pointer so >> whatever. Can the author just push a patch already? >> > > I did it just now, but as the author and maintainer of the code in > question you're the most adequate person to review and push patches for it. > Yes but the submitter has push access so I thought it best to leave it up to them to push.
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of James Almer > Sent: Friday, May 17, 2019 5:30 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH V2] avutil/tx: add check against (*ctx) > > On 5/16/2019 6:06 PM, Lynne wrote: > > May 16, 2019, 8:43 PM by george@nsup.org: > > > >> Lynne (12019-05-16): > >> > >>> I'm not, I still want the 2 checks. > >>> > >> > >> Arguments please. As I explained, the first check is harmful to > >> applications because it hides bug. > >> > > > > Nevermind, its not really possible to give av_tx_uninit() a NULL double > pointer so > > whatever. Can the author just push a patch already? > > I did it just now, but as the author and maintainer of the code in > question you're the most adequate person to review and push patches for it. After reading the discussion, I agree to use v1 to just crash if null pointer passed in and thanks James for approve and push the patch. And thank you all for the active discussion:) Ruiling > _______________________________________________ > 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".
diff --git a/libavutil/tx.c b/libavutil/tx.c index 934ef27c81..1690604040 100644 --- a/libavutil/tx.c +++ b/libavutil/tx.c @@ -697,7 +697,7 @@ static int gen_mdct_exptab(AVTXContext *s, int len4, double scale) av_cold void av_tx_uninit(AVTXContext **ctx) { - if (!ctx) + if (!ctx || !(*ctx)) return; av_free((*ctx)->pfatab);
ctx is a pointer to pointer here. Signed-off-by: Ruiling Song <ruiling.song@intel.com> --- libavutil/tx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)