[FFmpeg-devel] [PATCH 2/2] ffserver_config: replace strtod by av_strtod and correct undefined behavior

[Ganesh Ajjanagadde]

On Sun, Nov 15, 2015 at 8:39 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Sun, Nov 15, 2015 at 09:38:34AM -0500, Ganesh Ajjanagadde wrote:
>> On Sat, Nov 14, 2015 at 4:12 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Thu, Nov 12, 2015 at 09:46:05PM -0500, Ganesh Ajjanagadde wrote:
>> >> This uses av_strtod for added flexibility, and av_rint64_clip for ensuring that
>> >> no undefined behavior gets invoked.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> ---
>> >>  ffserver_config.c | 21 +++++----------------
>> >>  1 file changed, 5 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/ffserver_config.c b/ffserver_config.c
>> >> index 9fc1f00..443e71d 100644
>> >> --- a/ffserver_config.c
>> >> +++ b/ffserver_config.c
>> >> @@ -19,6 +19,7 @@
>> >>   */
>> >>
>> >>  #include <float.h>
>> >> +#include "libavutil/eval.h"
>> >>  #include "libavutil/opt.h"
>> >>  #include "libavutil/parseutils.h"
>> >>  #include "libavutil/avstring.h"
>> >> @@ -757,7 +758,7 @@ static int ffserver_parse_config_feed(FFServerConfig *config, const char *cmd,
>> >>          } else {
>> >>              WARNING("Truncate N syntax in configuration file is deprecated. "
>> >>                      "Use Truncate alone with no arguments.\n");
>> >> -            feed->truncate = strtod(arg, NULL);
>> >> +            feed->truncate = av_rint64_clip(av_strtod(arg, NULL), INT_MIN, INT_MAX);
>> >>          }
>> >>      } else if (!av_strcasecmp(cmd, "FileMaxSize")) {
>> >>          char *p1;
>> >> @@ -765,22 +766,10 @@ static int ffserver_parse_config_feed(FFServerConfig *config, const char *cmd,
>> >>
>> >>          ffserver_get_arg(arg, sizeof(arg), p);
>> >>          p1 = arg;
>> >> -        fsize = strtod(p1, &p1);
>> >> -        switch(av_toupper(*p1)) {
>> >> -        case 'K':
>> >> -            fsize *= 1024;
>> >> -            break;
>> >> -        case 'M':
>> >> -            fsize *= 1024 * 1024;
>> >> -            break;
>> >> -        case 'G':
>> >> -            fsize *= 1024 * 1024 * 1024;
>> >> -            break;
>> >> -        default:
>> >> +        fsize = av_strtod(p1, &p1);
>> >> +        if (!fsize || fabs(fsize) == HUGE_VAL)
>> >>              ERROR("Invalid file size: '%s'\n", arg);
>> >> -            break;
>> >> -        }
>> >> -        feed->feed_max_size = (int64_t)fsize;
>> >> +        feed->feed_max_size = av_rint64_clip(fsize, INT64_MIN, INT64_MAX);
>> >
>> > i think these should be range checked and causing errors or warnings
>> > if they are out of range
>> >
>> > if the user asks for value X and we cant do that then we should tell
>> > the user that we cant.
>>
>> The first one does not need to be, it is deprecated syntax and anyway
>> the only value it cares about is 0 versus nonzero.
>>
>> The second one can be range checked. Honestly though, I do not favor
>> adding cruft like this for minimal gain since for all practical
>> usages, no one will pass a value >= 2^64 in absolute value. For me,
>> what is important is avoiding the undefined behavior. If you or others
>> still feel that they need to be range checked, will do so.
>
> i belive checking user input for validity is important
> so the user knows if there is a problem and where it is instead of
> just something not working
> and its very hard to predict which check is not usefull
> there are a wide array of possible typos, from missing wrong or
> superflous seperators to all kinds of wrong chars, copy and paste
> errors, mistakenly duplicated values, ...

I am convinced of the checks. But the desired clip API is currently
unavailable as it is in avutil/internal. Will have to wait for a fix
for that before reposting...

>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Rewriting code that is poorly written but fully understood is good.
> Rewriting code that one doesnt understand is a sign that one is less smart
> then the original author, trying to rewrite it will not make it better.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>