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

[Ganesh Ajjanagadde]

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.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Everything should be made as simple as possible, but not simpler.
> -- Albert Einstein
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>