Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hello! On Thu, Oct 27, 2016 at 12:40:21AM +0800, 胡聪 (hucc) wrote: > Hello! > > On Wed, Oct 26, 2016 at 8:31PM, Maxim Dounin wrote: > > >On Wed, Oct 26, 2016 at 05:10:25PM +0800, 胡聪 (hucc) wrote: > > > >> VOD (video on demand) module which support requesting time range > >> also need the special function, so i would like to see that nginx-devel > >> add a function in Core, that would be helpful. Possible patch below. > > > >If you mean https://github.com/kaltura/nginx-vod-module, then I > >don't think it does. Quick look suggests that it uses > >milliseconds to specify times instead, and uses ngx_atoi() to > >parse them. It doesn't use strtod() at all, and the only place it > >uses ngx_atofp() now is to parse rate, and I don't think there are > >reasons to allow arbitrary numbers like in the mp4 case. > > > >The main problem with mp4 was that there were existing Flash > >clients nginx has to be compatible with, and some of them used to > >send incorrect start time specifications. It is highly unlikely > >that the same unique situation affects other modules. > > I don`t know how to make a more specific description because of my > poor english. What i mean about VOD is video-processing module in > CDN provider, this scenario usually be asked to provide parameter > parsing which support seconds and milliseconds. I am developing a > series of moudles which are a bit like nginx-vod-module and new > function will be used based on my experience. I just think the usage > of atofp() is very limited, maybe a function supporting optional mode > should be provided. Anyway, the point is whether other modules > need new function, i pointed out the scene i familiar with and hope > this helps. There is no problem with parsing seconds and milliseconds using ngx_atofp(). The problem with ngx_atofp() only appears if you have to support existing clients who already use invalid syntax. Unless you have to support such clients - you'd better use generic ngx_atofp() and reject attempts to use invalid syntax in the arguments. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hello! On Wed, Oct 26, 2016 at 8:31PM, Maxim Dounin wrote: >On Wed, Oct 26, 2016 at 05:10:25PM +0800, (hucc) wrote: > >> VOD (video on demand) module which support requesting time range >> also need the special function, so i would like to see that nginx-devel >> add a function in Core, that would be helpful. Possible patch below. > >If you mean https://github.com/kaltura/nginx-vod-module, then I >don't think it does. Quick look suggests that it uses >milliseconds to specify times instead, and uses ngx_atoi() to >parse them. It doesn't use strtod() at all, and the only place it >uses ngx_atofp() now is to parse rate, and I don't think there are >reasons to allow arbitrary numbers like in the mp4 case. > >The main problem with mp4 was that there were existing Flash >clients nginx has to be compatible with, and some of them used to >send incorrect start time specifications. It is highly unlikely >that the same unique situation affects other modules. I don`t know how to make a more specific description because of my poor english. What i mean about VOD is video-processing module in CDN provider, this scenario usually be asked to provide parameter parsing which support seconds and milliseconds. I am developing a series of moudles which are a bit like nginx-vod-module and new function will be used based on my experience. I just think the usage of atofp() is very limited, maybe a function supporting optional mode should be provided. Anyway, the point is whether other modules need new function, i pointed out the scene i familiar with and hope this helps. Regards, -hucc___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hello! On Wed, Oct 26, 2016 at 05:10:25PM +0800, 胡聪 (hucc) wrote: > On Wed, Oct 26, 2016 3:34 AM, Maxim Dounin wrote: > > >Using strtod() should be avoided as it has other problems, see > >https://trac.nginx.org/nginx/ticket/475. That is, an additional > >variant of ngx_atofp() is a way to go. > > > > > >Last time we've looked into this, we've stumbled upon selecting a > >proper name for the additional function. Looking into this again > >I tend to think that proper solution would be to use a special > >function withing the ngx_http_mp4_module itself. Patch below. > > > VOD (video on demand) module which support requesting time range > also need the special function, so i would like to see that nginx-devel > add a function in Core, that would be helpful. Possible patch below. If you mean https://github.com/kaltura/nginx-vod-module, then I don't think it does. Quick look suggests that it uses milliseconds to specify times instead, and uses ngx_atoi() to parse them. It doesn't use strtod() at all, and the only place it uses ngx_atofp() now is to parse rate, and I don't think there are reasons to allow arbitrary numbers like in the mp4 case. The main problem with mp4 was that there were existing Flash clients nginx has to be compatible with, and some of them used to send incorrect start time specifications. It is highly unlikely that the same unique situation affects other modules. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hello!
On Wed, Oct 26, 2016 3:34 AM, Maxim Dounin wrote:
>Using strtod() should be avoided as it has other problems, see
>https://trac.nginx.org/nginx/ticket/475. That is, an additional
>variant of ngx_atofp() is a way to go.
>
>
>Last time we've looked into this, we've stumbled upon selecting a
>proper name for the additional function. Looking into this again
>I tend to think that proper solution would be to use a special
>function withing the ngx_http_mp4_module itself. Patch below.
Sorry, my mistake, last patch lost some codes when i transfer codes
from Git to Mercurial.
VOD (video on demand) module which support requesting time range
also need the special function, so i would like to see that nginx-devel
add a function in Core, that would be helpful. Possible patch below.
# HG changeset patch
# User hucongcong
# Date 1477473854 -28800
# Wed Oct 26 17:24:14 2016 +0800
# Node ID 652843788cb08c18dcbc6bab2857a228093767e4
# Parent 56d6bfe6b609c565a9f500bde573fd9b488ff960
Core: add ngx_atosp().
This allows to correctly parse "start" and "end" arguments without
null-termination (ticket #475), and also fixes rounding errors observed
with strtod() when using i387 instructions.
diff -r 56d6bfe6b609 -r 652843788cb0 src/core/ngx_string.c
--- a/src/core/ngx_string.c Fri Oct 21 16:28:39 2016 +0300
+++ b/src/core/ngx_string.c Wed Oct 26 17:24:14 2016 +0800
@@ -926,7 +926,9 @@
}
-/* parse a fixed point number, e.g., ngx_atofp("10.5", 4, 2) returns 1050 */
+/* parse a fixed point number, e.g., ngx_atofp("10.5", 4, 2) returns 1050.
+ * can be replaced by ngx_atosp(line, n, point, 1).
+ */
ngx_int_t
ngx_atofp(u_char *line, size_t n, size_t point)
@@ -982,6 +984,76 @@
}
+/* parse a fixed point number and do bounds check when strict is true, e.g.,
+ * ngx_atosp("12.2193", 3, 0, 0) returns 12,
+ * ngx_atosp("12.2193", 3, 0, 1) returns NGX_ERROR,
+ * ngx_atosp("12.2193", 6, 3, 1) returns 12219,
+ * ngx_atosp("12.2193", 7, 3, 1) returns NGX_ERROR,
+ * ngx_atosp("12.2193", 7, 3, 0) returns 12219.
+ */
+
+ngx_int_t
+ngx_atosp(u_char *line, size_t n, size_t point, unsigned strict)
+{
+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;
+}
+
+if (strict && point == 0) {
+return NGX_ERROR;
+}
+
+dot = 1;
+continue;
+}
+
+if (*line < '0' || *line > '9') {
+return NGX_ERROR;
+}
+
+if (dot && point == 0) {
+if (strict) {
+return NGX_ERROR;
+}
+
+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;
+}
+
+
ssize_t
ngx_atosz(u_char *line, size_t n)
{
diff -r 56d6bfe6b609 -r 652843788cb0 src/core/ngx_string.h
--- a/src/core/ngx_string.h Fri Oct 21 16:28:39 2016 +0300
+++ b/src/core/ngx_string.h Wed Oct 26 17:24:14 2016 +0800
@@ -171,6 +171,7 @@
ngx_int_t ngx_atoi(u_char *line, size_t n);
ngx_int_t ngx_atofp(u_char *line, size_t n, size_t point);
+ngx_int_t ngx_atosp(u_char *line, size_t n, size_t point, unsigned strict);
ssize_t ngx_atosz(u_char *line, size_t n);
off_t ngx_atoof(u_char *line, size_t n);
time_t ngx_atotm(u_char *line, size_t n);
diff -r 56d6bfe6b609 -r 652843788cb0 src/http/modules/ngx_http_mp4_module.c
--- a/src/http/modules/ngx_http_mp4_module.cFri Oct 21 16:28:39 2016 +0300
+++ b/src/http/modules/ngx_http_mp4_module.cWed Oct 26 17:24:14 2016 +0800
@@ -537,26 +537,15 @@
/*
* A Flash player may send start value with a lot of digits
- * after dot so strtod() is used instead of atofp(). NaNs and
- * infinities become negative numbers after (int) conversion.
+ * after dot so ngx_atosp() is used instead of ngx_atofp().
*/
-ngx_set_errno(0);
-start = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-start = -1;
-}
+start = ngx_atosp(value.data, value.len, 3, 0);
}
if (ngx_http_arg(r, (u_char *) "end", 3, &value) == NGX_OK) {
-ngx_set_errno(0);
-end = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if
Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hello!
On Wed, Oct 26, 2016 3:34 AM, Maxim Dounin wrote:
>Using strtod() should be avoided as it has other problems, see
>https://trac.nginx.org/nginx/ticket/475. That is, an additional
>variant of ngx_atofp() is a way to go.
>
>
>Last time we've looked into this, we've stumbled upon selecting a
>proper name for the additional function. Looking into this again
>I tend to think that proper solution would be to use a special
>function withing the ngx_http_mp4_module itself. Patch below.
VOD (video on demand) module which support requesting time range
also need the special function, so i would like to see that nginx-devel
add a function in Core, that would be helpful. Possible patch below.
# HG changeset patch
# User hucongcong
# Date 1477468236 -28800
# Wed Oct 26 15:50:36 2016 +0800
# Node ID 6376b8da3689011dfc8805381970f9ea2251c0a6
# Parent 56d6bfe6b609c565a9f500bde573fd9b488ff960
Core: add ngx_atosp().
This allows to correctly parse "start" and "end" arguments without
null-termination (ticket #475), and also fixes rounding errors observed
with strtod() when using i387 instructions.
diff -r 56d6bfe6b609 -r 6376b8da3689 src/core/ngx_string.c
--- a/src/core/ngx_string.c Fri Oct 21 16:28:39 2016 +0300
+++ b/src/core/ngx_string.c Wed Oct 26 15:50:36 2016 +0800
@@ -926,7 +926,9 @@
}
-/* parse a fixed point number, e.g., ngx_atofp("10.5", 4, 2) returns 1050 */
+/* parse a fixed point number, e.g., ngx_atofp("10.5", 4, 2) returns 1050.
+ * can be replaced by ngx_atosp(line, n, point, 1).
+ */
ngx_int_t
ngx_atofp(u_char *line, size_t n, size_t point)
@@ -982,6 +984,76 @@
}
+/* parse a fixed point number and do bounds check when strict is true, e.g.,
+ * ngx_atosp("12.2193", 3, 0, 0) returns 12,
+ * ngx_atosp("12.2193", 3, 0, 1) returns NGX_ERROR,
+ * ngx_atosp("12.2193", 6, 3, 1) returns 12219,
+ * ngx_atosp("12.2193", 7, 3, 1) returns NGX_ERROR,
+ * ngx_atosp("12.2193", 7, 3, 0) returns 12219.
+ */
+
+ngx_int_t
+ngx_atosp(u_char *line, size_t n, size_t point, unsigned strict)
+{
+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;
+}
+
+if (strict && point == 0) {
+return NGX_ERROR;
+}
+
+dot = 1;
+continue;
+}
+
+if (*line < '0' || *line > '9') {
+return NGX_ERROR;
+}
+
+if (dot && point == 0) {
+if (strict) {
+return NGX_ERROR;
+}
+
+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;
+}
+
+
ssize_t
ngx_atosz(u_char *line, size_t n)
{
diff -r 56d6bfe6b609 -r 6376b8da3689 src/http/modules/ngx_http_mp4_module.c
--- a/src/http/modules/ngx_http_mp4_module.cFri Oct 21 16:28:39 2016 +0300
+++ b/src/http/modules/ngx_http_mp4_module.cWed Oct 26 15:50:36 2016 +0800
@@ -537,26 +537,15 @@
/*
* A Flash player may send start value with a lot of digits
- * after dot so strtod() is used instead of atofp(). NaNs and
- * infinities become negative numbers after (int) conversion.
+ * after dot so ngx_atosp() is used instead of ngx_atofp().
*/
-ngx_set_errno(0);
-start = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-start = -1;
-}
+start = ngx_atosp(value.data, value.len, 3, 0);
}
if (ngx_http_arg(r, (u_char *) "end", 3, &value) == NGX_OK) {
-ngx_set_errno(0);
-end = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-end = -1;
-}
+end = ngx_atosp(value.data, value.len, 3, 0);
if (end > 0) {
if (start < 0) {___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hello!
On Wed, Oct 19, 2016 at 02:43:24PM +0800, 胡聪 (hucc) wrote:
> 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?
Using strtod() should be avoided as it has other problems, see
https://trac.nginx.org/nginx/ticket/475. That is, an additional
variant of ngx_atofp() is a way to go.
Last time we've looked into this, we've stumbled upon selecting a
proper name for the additional function. Looking into this again
I tend to think that proper solution would be to use a special
function withing the ngx_http_mp4_module itself. Patch below.
# HG changeset patch
# User Maxim Dounin
# Date 1477416871 -10800
# Tue Oct 25 20:34:31 2016 +0300
# Node ID 16dae7d3b770c63dc16ac06312a66c1bafc3308f
# Parent 56d6bfe6b609c565a9f500bde573fd9b488ff960
Mp4: introduced custom version of ngx_atofp().
This allows to correctly parse "start" and "end" arguments without
null-termination (ticket #475), and also fixes rounding errors observed
with strtod() when using i387 instructions.
diff --git a/src/http/modules/ngx_http_mp4_module.c
b/src/http/modules/ngx_http_mp4_module.c
--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -216,6 +216,7 @@ typedef struct {
static ngx_int_t ngx_http_mp4_handler(ngx_http_request_t *r);
+static ngx_int_t ngx_http_mp4_atofp(u_char *line, size_t n, size_t point);
static ngx_int_t ngx_http_mp4_process(ngx_http_mp4_file_t *mp4);
static ngx_int_t ngx_http_mp4_read_atom(ngx_http_mp4_file_t *mp4,
@@ -537,26 +538,15 @@ ngx_http_mp4_handler(ngx_http_request_t
/*
* A Flash player may send start value with a lot of digits
- * after dot so strtod() is used instead of atofp(). NaNs and
- * infinities become negative numbers after (int) conversion.
+ * after dot so a custom function is used instead of ngx_atofp().
*/
-ngx_set_errno(0);
-start = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-start = -1;
-}
+start = ngx_http_mp4_atofp(value.data, value.len, 3);
}
if (ngx_http_arg(r, (u_char *) "end", 3, &value) == NGX_OK) {
-ngx_set_errno(0);
-end = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-end = -1;
-}
+end = ngx_http_mp4_atofp(value.data, value.len, 3);
if (end > 0) {
if (start < 0) {
@@ -687,6 +677,62 @@ ngx_http_mp4_handler(ngx_http_request_t
static ngx_int_t
+ngx_http_mp4_atofp(u_char *line, size_t n, size_t point)
+{
+ngx_int_t value, cutoff, cutlim;
+ngx_uint_t dot;
+
+/* same as ngx_atofp(), but allows additional digits */
+
+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 (point == 0) {
+break;
+}
+
+if (*line == '.') {
+if (dot) {
+return NGX_ERROR;
+}
+
+dot = 1;
+continue;
+}
+
+if (*line < '0' || *line > '9') {
+return NGX_ERROR;
+}
+
+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;
+}
+
+
+static ngx_int_t
ngx_http_mp4_process(ngx_http_mp4_file_t *mp4)
{
off_t start_offset, end_offset, adjustment;
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
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";;
Send time: Wednesday, Oct 19, 2016 2:25 AM
To: "nginx-devel";
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
> # 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@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
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
> # 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@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
[patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
Hi, all I've sent a patch. http://mailman.nginx.org/pipermail/nginx-devel/2016-October/008932.html that solves precision problem of ngx_http_mp4_module. Did any one noticed this patch. Downstream cache system receives duplicate data by sending start&end parameters which are analyzed from moov box and aligned with the GOP. PotPlayer becomes not fluent when playing hls stream which is converted from the mp4 (duplicate data). Nginx can not quietly change the parameter value, and send non-requested data to clients or player. So the patch is helpful. Regards, -hucc___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
[patch][bugfix]Http mp4: replace strtod() with improved ngx_atofp() because precision problem.
# HG changeset patch
# User hucongcong
# 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 == '.') {
diff -r 7cdf69d012f0 -r 93bad8b82169 src/http/modules/ngx_http_mp4_module.c
--- a/src/http/modules/ngx_http_mp4_module.cTue Oct 11 18:03:01 2016 +0300
+++ b/src/http/modules/ngx_http_mp4_module.cThu Oct 13 15:12:51 2016 +0800
@@ -535,28 +535,14 @@
if (ngx_http_arg(r, (u_char *) "start", 5, &value) == NGX_OK) {
-/*
- * A Flash player may send start value with a lot of digits
- * after dot so strtod() is used instead of atofp(). NaNs and
- * infinities become negative numbers after (int) conversion.
- */
-
-ngx_set_errno(0);
-start = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-start = -1;
-}
+/* start = -1 when NGX_ERROR returned by ngx_atofp */
+
+start = ngx_atofp(value.data, value.len, 3);
}
if (ngx_http_arg(r, (u_char *) "end", 3, &value) == NGX_OK) {
-ngx_set_errno(0);
-end = (int) (strtod((char *) value.data, NULL) * 1000);
-
-if (ngx_errno != 0) {
-end = -1;
-}
+end = ngx_atofp(value.data, value.len, 3);
if (end > 0) {
if (start < 0) {
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
