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

Michael Niedermayer michael at niedermayer.cc
Mon Nov 16 02:39:42 CET 2015


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, ...


[...]
-- 
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151116/5d4e709c/attachment.sig>


More information about the ffmpeg-devel mailing list