[patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.

[胡聪 (hucc)]

Hello!


Thanks for reminding me! Illegal character should be checked
also error detection in configration parser is needed.


Considering that ngx_atofp() was used in some 3rd party modules,
Would nginx-devel accept a new function like ngx_ato??() or
lround(strtod()), or just let strtod() alone?


#lround(strtod())
 
--- a/src/http/modules/ngx_http_mp4_module.c
 
+++ b/src/http/modules/ngx_http_mp4_module.c
 
@@ -542,7 +542,7 @@ ngx_http_mp4_handler(ngx_http_request_t *r)
 
              */
 
 
 
             ngx_set_errno(0);
 
-            start = (int) (strtod((char *) value.data, NULL) * 1000);
 
+            start = (int) (lround(strtod((char *) value.data, NULL) * 1000));
 
 
 
             if (ngx_errno != 0) {
 
                 start = -1;
 
@@ -552,7 +552,7 @@ ngx_http_mp4_handler(ngx_http_request_t *r)
 
         if (ngx_http_arg(r, (u_char *) "end", 3, &value) == NGX_OK) {
 
 
 
             ngx_set_errno(0);
 
-            end = (int) (strtod((char *) value.data, NULL) * 1000);
 
+            end = (int) (lround(strtod((char *) value.data, NULL) * 1000));



#new function
 
+ngx_int_t
 
+ngx_atopi(u_char *line, size_t n, size_t point)
 
+{
 
+    ngx_int_t   value, cutoff, cutlim;
 
+    ngx_uint_t  dot;
 
+
 
+    if (n == 0) {
 
+        return NGX_ERROR;
 
+    }
 
+
 
+    cutoff = NGX_MAX_INT_T_VALUE / 10;
 
+    cutlim = NGX_MAX_INT_T_VALUE % 10;
 
+
 
+    dot = 0;
 
+
 
+    for (value = 0; n--; line++) {
 
+
 
+        if (*line == '.') {
 
+            if (dot) {
 
+                return NGX_ERROR;
 
+            }
 
+
 
+            dot = 1;
 
+            continue;
 
+        }
 
+
 
+        if (*line < '0' || *line > '9') {
 
+            return NGX_ERROR;
 
+        }
 
+
 
+        if (dot && point == 0) {
 
+            continue;
 
+        }
 
+
 
+        if (value >= cutoff && (value > cutoff || *line - '0' > cutlim)) {
 
+            return NGX_ERROR;
 
+        }
 
+
 
+        value = value * 10 + (*line - '0');
 
+        point -= dot;
 
+    }
 
+
 
+    while (point--) {
 
+        if (value > cutoff) {
 
+            return NGX_ERROR;
 
+        }
 
+
 
+        value = value * 10;
 
+    }
 
+
 
+    return value;
 
+}

------------------ Original ------------------
From:  "Maxim Dounin";<mdounin at mdounin.ru>;
Send time: Wednesday, Oct 19, 2016 2:25 AM
To: "nginx-devel"<nginx-devel at nginx.org>; 

Subject:  Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.



Hello!

On Thu, Oct 13, 2016 at 04:45:23PM +0800, 月耳 wrote:

> # HG changeset patch
> # User hucongcong <hucong.c at foxmail.com>
> # Date 1476342771 -28800
> #      Thu Oct 13 15:12:51 2016 +0800
> # Node ID 93bad8b82169245db6d4fe4e6b6c823221ee6a38
> # Parent  7cdf69d012f02a80c94d930b29c71847e203e4d6
> Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
> 
> function prototype is ngx_atofp(u_char *line, size_t n, size_t point).
> use case based on the old version:
> ngx_atofp("12.2193", 7, 0) returns NGX_ERROR,
> ngx_atofp("12.2193", 7, 2) returns NGX_ERROR.
> now, allow point = 0 or point less than the number of fractional digits.
> ngx_atofp("12.2193", 7, 0) returns 12,
> ngx_atofp("12.2193", 7, 2) returns 1221.
> retained all the required or right feature at the same time, e.g.,
> ngx_atofp("10.5", 4, 2) returns 1050.
> 
> deprecated strtod() in ngx_http_mp4_module, since the precision problem,
> which was metioned in http://stackoverflow.com/questions/18361261, e.g.,
> (int) (strtod((char *) "32.480", NULL) * 1000) returns 32479.
> another way to solve this problem is like this round(strtod()), e.g.,
> (int) round((strtod((char *) "32.480", NULL) * 1000)) returns 32480,
> but its not necessary, since we have a better ngx_atofp().
> 
> diff -r 7cdf69d012f0 -r 93bad8b82169 src/core/ngx_string.c
> --- a/src/core/ngx_string.c	Tue Oct 11 18:03:01 2016 +0300
> +++ b/src/core/ngx_string.c	Thu Oct 13 15:12:51 2016 +0800
> @@ -945,8 +945,8 @@
>  
>      for (value = 0; n--; line++) {
>  
> -        if (point == 0) {
> -            return NGX_ERROR;
> +        if (dot && point == 0) {
> +            break;
>          }
>  
>          if (*line == '.') {

With such a change it won't be possible to detect errors in other 
places where ngx_atofp() is used.  In particular, something like

    split_clients $remote_addr $foo {
        0.015% 1;
        *      2;
    }

will silently discard trailing 5.  Moreover, even something like

    split_clients $remote_addr $foo {
        0.01foobar% 1;
        *           2;
    }

will be silently accepted.

[...]

-- 
Maxim Dounin
http://nginx.org/

_______________________________________________
nginx-devel mailing list
nginx-devel at nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20161019/e9ae75d8/attachment.html>