A NULL check in av_parse_video_rate() would certainly not be a bad idea. It wouldn't (on its own) restore the NULL input robustness to set_string_video_rate() though, as any error value returned from av_parse_video_rate() would result in val being logged using %s, which is the whole problem that a500b975 was aiming to solve.
av_parse_video_rate() is also called from a lot more places (most of which already have an explicit NULL check immediately before the call), so changing the behaviour of that function by adding a NULL check potentially has a lot more knock-on effects than simply restoring the previously existing robustness to set_string_video_rate(), which is also in keeping with the other av_parse_video_rate() callsites. You say that the caller 'should not' pass NULL to set_string_video_rate(). Absolutely, agreed, but the point of checks is to handle unexpected situations. If someone does pass NULL, as of a500b975 the response will be undefined behaviour, where previously it would have been cleanly handled with an explicit error code. I'd contend that this is a bad response. Thanks Jack On Sun, Aug 2, 2020 at 11:28 PM Michael Niedermayer <mich...@niedermayer.cc> wrote: > On Sun, Aug 02, 2020 at 08:40:27PM +0100, Jack Haughton wrote: > > Hello, > > > > Apologies for the delay in replying. This patch is not to address a > > specific problem that currently exists, but rather to restore the > > robustness to invalid input that previously existed in this function. The > > motivation for the patch is that we would like to merge a500b975 into our > > local tree to gain support for gcc 10, but there are concerns about the > > removal of the null check. av_parse_video_rate() passes its argument > > directly to strcmp, which would cause undefined behaviour if the argument > > was NULL. > > now it makes it even less sense to me. > av_parse_video_rate() doesnt support a NULL argument before or after your > patch, its undefined behaviour either way. > what you change is a private function using that public function. > > Now if you changed the public function to do something specific with a NULL > argument then that would be more robust maybe but not when its done to a > single caller which should not pass NULL there > > thx > > > > > > > Thanks, > > Jack > > > > On Fri, Jul 31, 2020 at 8:31 PM Michael Niedermayer > <mich...@niedermayer.cc> > > wrote: > > > > > Hi > > > > > > On Fri, Jul 31, 2020 at 03:53:56PM +0100, Jack Haughton wrote: > > > > Commit a500b975 removed NULL input handling from this function, > > > > moving the check higher up the call tree in one branch. However, > > > > there is another call to set_string_video_rate() which may pass > > > > NULL, and future users of the function may not be clear that > > > > a NULL check is required. This patch restores the NULL check to > > > > avoid these problems. > > > > > > Does this affect something else than the seting based on defaults ? > > > because the defaults should probably be valid values > > > or if you disagree, iam curious where the default would be > intentionally > > > invalid > > > > > > thx > > > > > > [...] > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > > > Breaking DRM is a little like attempting to break through a door even > > > though the window is wide open and the only thing in the house is a > bunch > > > of things you dont want and which you would get tomorrow for free > anyway > > > > > > > > > -- > > > > Argon Design Ltd., Registration No. 06977690, a Broadcom Inc. company > > > > Registered in England with registered office at St John's Innovation > > Centre, Cowley Road, Cambridge, CB4 0WS > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > It is what and why we do it that matters, not just one of them. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". -- Argon Design Ltd., Registration No. 06977690, a Broadcom Inc. company Registered in England with registered office at St John's Innovation Centre, Cowley Road, Cambridge, CB4 0WS _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
