On Sun, Nov 15, 2015 at 8:39 PM, Michael Niedermayer
<mich...@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
>> <mich...@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 <gajjanaga...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
