BCmath fix for validation arguments #4092

vv12131415 · 2019-04-30T14:46:32Z

I've added only todo for it here since I'm not C dev.

my concept looks like this, but some why it doesn't work as expected.

static void php_str2num(bc_num *num, char *str)
{
        zend_string *regex  = zend_string_init("/^[+-]?[0-9]+(.[0-9]+)?$/", sizeof("/^[+-]?[0-9]+(.[0-9]+)?$/") - 1, 0);
        pcre_cache_entry *pce;                          /* Compiled regex */
        zval *subpats = NULL;                           /* Parts (not used) */
        int global = 0;
        zval return_value;

        ZVAL_NULL(&return_value);

        if ((pce = pcre_get_compiled_regex_cache(regex))== NULL) {
                zend_string_release(regex);
                php_error_docref(NULL, E_WARNING, "bcmath function argument is not well formatted");
        }

        zend_string_release(regex);
        php_pcre_match_impl(pce, zend_string_init(str, sizeof(str) -1, 0), &return_value, subpats, global,0, Z_L(0), Z_L(0));

        if (!Z_LVAL(return_value)) {
                php_error_docref(NULL, E_WARNING, "bcmath function argument is not well formatted");
        }

        char *p;

        if (!(p = strchr(str, '.'))) {
                bc_str2num(num, str, 0);
                return;
        }

        bc_str2num(num, str, strlen(p+1));
}

I think this should be easy for people who know how to do C right way.

Here are threads from internals mailing list to get what I want to implement
https://externals.io/message/104295#104295
https://externals.io/message/104374#105461

cmb69 · 2019-04-30T15:38:57Z

It seems to me that a regex would be overkill, since bc_str2num() already does the necessary checks. We just could make it signal if the str is not acceptable, and to catch this in all its callers, e.g.

 ext/bcmath/bcmath.c                | 16 ++++++++++++----
 ext/bcmath/libbcmath/src/bcmath.h  |  2 +-
 ext/bcmath/libbcmath/src/str2num.c |  6 ++++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/ext/bcmath/bcmath.c b/ext/bcmath/bcmath.c
index cc9d901fdf..84735b5e5d 100644
--- a/ext/bcmath/bcmath.c
+++ b/ext/bcmath/bcmath.c
@@ -198,11 +198,15 @@ static void php_str2num(bc_num *num, char *str)
 	char *p;
 
 	if (!(p = strchr(str, '.'))) {
-		bc_str2num(num, str, 0);
+		if (bc_str2num(num, str, 0)) {
+			php_error_docref(NULL, E_WARNING, "bcmath function argument is not well formatted");
+		}
 		return;
 	}
 
-	bc_str2num(num, str, strlen(p+1));
+	if (bc_str2num(num, str, strlen(p+1))) {
+		php_error_docref(NULL, E_WARNING, "bcmath function argument is not well formatted");
+	}
 }
 /* }}} */
 
@@ -527,8 +531,12 @@ PHP_FUNCTION(bccomp)
 	bc_init_num(&first);
 	bc_init_num(&second);
 
-	bc_str2num(&first, ZSTR_VAL(left), scale);
-	bc_str2num(&second, ZSTR_VAL(right), scale);
+	if (bc_str2num(&first, ZSTR_VAL(left), scale)) {
+		php_error_docref(NULL, E_WARNING, "bcmath function argument is not well formatted");
+	}
+	if (bc_str2num(&second, ZSTR_VAL(right), scale)) {
+		php_error_docref(NULL, E_WARNING, "bcmath function argument is not well formatted");
+	}
 	RETVAL_LONG(bc_compare(first, second));
 
 	bc_free_num(&first);
diff --git a/ext/bcmath/libbcmath/src/bcmath.h b/ext/bcmath/libbcmath/src/bcmath.h
index 78868edcb3..1297a3279c 100644
--- a/ext/bcmath/libbcmath/src/bcmath.h
+++ b/ext/bcmath/libbcmath/src/bcmath.h
@@ -108,7 +108,7 @@ _PROTOTYPE(bc_num bc_copy_num, (bc_num num));
 
 _PROTOTYPE(void bc_init_num, (bc_num *num));
 
-_PROTOTYPE(void bc_str2num, (bc_num *num, char *str, int scale));
+_PROTOTYPE(int bc_str2num, (bc_num *num, char *str, int scale));
 
 _PROTOTYPE(zend_string *bc_num2str_ex, (bc_num num, int scale));
 
diff --git a/ext/bcmath/libbcmath/src/str2num.c b/ext/bcmath/libbcmath/src/str2num.c
index f38d341570..6cca4a0760 100644
--- a/ext/bcmath/libbcmath/src/str2num.c
+++ b/ext/bcmath/libbcmath/src/str2num.c
@@ -40,7 +40,7 @@
 
 /* Convert strings to bc numbers.  Base 10 only.*/
 
-void
+int
 bc_str2num (bc_num *num, char *str, int scale)
 {
   int digits, strscale;
@@ -63,7 +63,7 @@ bc_str2num (bc_num *num, char *str, int scale)
   if ((*ptr != '\0') || (digits+strscale == 0))
     {
       *num = bc_copy_num (BCG(_zero_));
-      return;
+      return str[0] != '0' || str[1] != '\0';
     }
 
   /* Adjust numbers and allocate storage and initialize fields. */
@@ -108,4 +108,6 @@ bc_str2num (bc_num *num, char *str, int scale)
 
   if (bc_is_zero (*num))
     (*num)->n_sign = PLUS;
+
+  return 0;
 }

Note that this would allow strings like .123 as before.

vv12131415 · 2019-04-30T21:27:06Z

@cmb69 yeah, works well!

vv12131415 · 2019-04-30T22:41:36Z

I've found that I've failed some tests somehow.
Here are they

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Bug #63217 (Constant numeric strings become integers when used as ArrayAccess offset) [Zend/tests/bug63217.phpt]
DCE 005: Elimination of assignment to non-escaping objects (can't remove NEW yet) [ext/opcache/tests/opt/dce_005.phpt]
SCCP 010: Conditional Constant Propagation of non-escaping object properties [ext/opcache/tests/opt/sccp_010.phpt]
SCCP 011: Conditional Constant Propagation of non-escaping object properties [ext/opcache/tests/opt/sccp_011.phpt]
SCCP 012: Conditional Constant Propagation of non-escaping object properties [ext/opcache/tests/opt/sccp_012.phpt]
Test posix_getgrnam() function : basic functionality [ext/posix/tests/posix_getgrnam_basic.phpt]
Bug #69900 Commandline input/output weird behaviour with STDIO [ext/standard/tests/streams/proc_open_bug69900.phpt]
=====================================================================

those 2 are from PHP-7.4 branch

Test posix_getgrnam() function : basic functionality [ext/posix/tests/posix_getgrnam_basic.phpt]
Bug #69900 Commandline input/output weird behaviour with STDIO [ext/standard/tests/streams/proc_open_bug69900.phpt]

I've tested the Bug #63217 (Constant numeric strings become integers when used as ArrayAccess offset) [Zend/tests/bug63217.phpt] manually and it works, but it fails with make test don't know how that works.

nikic · 2019-05-01T08:16:27Z

@vladyslavstartsev Can you paste the contents of ext/opcache/tests/opt/sccp_010.diff (and maybe other .diff files) somewhere?

vv12131415 · 2019-05-01T12:24:40Z

@nikic
https://gist.github.com/vladyslavstartsev/a0c781e87c14248beceeff34af509fb9

is that good enough?

ext/bcmath/bcmath.c

ext/bcmath/tests/bug60377.phpt

nikic · 2019-05-07T12:19:08Z

@vladyslavstartsev It's not clear to me what is causing those test failures. My best guess is that an incorrect opcache.so gets loaded. In any case they are unrelated to your changes here.

vv12131415 · 2019-05-11T23:00:10Z

@nikic done all the things!

nikic · 2019-05-13T08:39:22Z

ext/bcmath/libbcmath/src/str2num.c

@@ -62,7 +62,7 @@ bc_str2num (bc_num *num, char *str, int scale)
  if ((*ptr != '\0') || (digits+strscale == 0))
    {
      *num = bc_copy_num (BCG(_zero_));
-      return;
+      return str[0] == '0' || str[1] == '\0';


Shouldn't the || here be a &&? And then it will not handle leading whitespace right. I think the right thing to do here might be return *ptr == '\0'.

I agree that we shouldn't throw a warning if there is leading whitespace (although the proposed regexp would not allow leading whitespace).

@nikic somehow the return *ptr == '\0' does not work, I'm still getting warnings on echo bcadd(" 0", "2"), "\n"; and echo bccomp(" 0", "2"), "\n";

Ah! As it is (and was), bc_tr2num() doesn't accept leading whitespace at all. We could change this like so:

ext/bcmath/libbcmath/src/str2num.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ext/bcmath/libbcmath/src/str2num.c b/ext/bcmath/libbcmath/src/str2num.c index 7fdccd15d6..8e20c6ec36 100644 --- a/ext/bcmath/libbcmath/src/str2num.c +++ b/ext/bcmath/libbcmath/src/str2num.c @@ -54,6 +54,7 @@ bc_str2num (bc_num *num, char *str, int scale) digits = 0; strscale = 0; zero_int = FALSE; + while (*ptr == ' ' || *ptr == '\t' || *ptr == '\n' || *ptr == '\r' || *ptr == '\v' || *ptr == '\f') ptr++; /* Skip leading whitespace. */ if ( (*ptr == '+') || (*ptr == '-')) ptr++; /* Sign */ while (*ptr == '0') ptr++; /* Skip leading zeros. */ while (isdigit((int)*ptr)) ptr++, digits++; /* digits */ @@ -76,6 +77,7 @@ bc_str2num (bc_num *num, char *str, int scale) /* Build the whole number. */ ptr = str; + while (*ptr == ' ' || *ptr == '\t' || *ptr == '\n' || *ptr == '\r' || *ptr == '\v' || *ptr == '\f') ptr++; /* Skip leading whitespace. */ if (*ptr == '-') { (*num)->n_sign = MINUS;

Anyhow, the test has to be fixed to work on other systems as well; currently, it expects hard-coded absolute path names. See how that's solved in bcpow_errror1.phpt and the documentation of the EXPECTF section.

@cmb69 fixed tests and leading white space char.
Seams to be done with all the things

ext/bcmath/tests/str2num_formatting.phpt

vv12131415 · 2019-05-13T23:04:00Z

ok,
the --DESCRIPTION-- thing is fixed
added yet another rows to tests
but as I said here #4092 (comment) somehow it is still throws warnings

cmb69

Thanks! LGTM.

nikic · 2019-05-14T10:39:06Z

Sorry about the confusion with the whitespace, I incorrectly assumed it was supported. I'm not totally sure if we really should add support for it if it's not there already, especially given the discussion in https://externals.io/message/104594. Instead of testing " 0" we could test something like "+0" to make sure the "non-trivial" zero case is handled correctly.

cmb69 · 2019-05-14T12:14:58Z

@nikic yeah, might be better to be conservative here.

Co-Authored-By: Nikita Popov <[email protected]> Co-Authored-By: Christoph M. Becker <[email protected]>

vv12131415 · 2019-05-14T12:36:14Z

Sorry about the confusion with the whitespace, I incorrectly assumed it was supported. I'm not totally sure if we really should add support for it if it's not there already, especially given the discussion in https://externals.io/message/104594. Instead of testing " 0" we could test something like "+0" to make sure the "non-trivial" zero case is handled correctly.

@nikic I've updated the code to throw warnings on " 0", but no warnings on "+0"/"-0"

nikic

Thanks!

nikic · 2019-05-14T13:05:24Z

Merged as a07d422 into 7.4. Thanks!

vv12131415 force-pushed the bcmath-fix branch from f80c8f7 to fa38e3e Compare April 30, 2019 21:25

vv12131415 force-pushed the bcmath-fix branch from fa38e3e to 3d51ec2 Compare April 30, 2019 22:38

vv12131415 marked this pull request as ready for review April 30, 2019 22:43

nikic reviewed May 7, 2019

View reviewed changes

ext/bcmath/bcmath.c Outdated Show resolved Hide resolved

ext/bcmath/bcmath.c Outdated Show resolved Hide resolved

ext/bcmath/tests/bug60377.phpt Show resolved Hide resolved

nikic mentioned this pull request May 7, 2019

Add other test cases for bcadd and bccomp #4110

Closed

vv12131415 force-pushed the bcmath-fix branch from 5dfe922 to a3460e5 Compare May 11, 2019 22:59

nikic reviewed May 13, 2019

View reviewed changes

ext/bcmath/tests/str2num_formatting.phpt Show resolved Hide resolved

nikic reviewed May 13, 2019

View reviewed changes

ext/bcmath/tests/str2num_formatting.phpt Outdated Show resolved Hide resolved

vv12131415 force-pushed the bcmath-fix branch from a3460e5 to 7272440 Compare May 13, 2019 23:01

vv12131415 force-pushed the bcmath-fix branch from 7272440 to 39fd382 Compare May 14, 2019 09:06

cmb69 approved these changes May 14, 2019

View reviewed changes

cmb69 requested a review from nikic May 14, 2019 09:15

cmb69 added the Waiting on Review label May 14, 2019

implement bcmath fix to warn about non well-formed arguments

d478b1f

Co-Authored-By: Nikita Popov <[email protected]> Co-Authored-By: Christoph M. Becker <[email protected]>

vv12131415 force-pushed the bcmath-fix branch from 39fd382 to d478b1f Compare May 14, 2019 12:34

nikic approved these changes May 14, 2019

View reviewed changes

nikic added Feature and removed Waiting on Review labels May 14, 2019

nikic closed this May 14, 2019

This was referenced May 25, 2019

Rules for bcmath functions phpstan/phpstan#1827

Closed

[PHP 7.4] polish upcoming features accoring to nikita's talk rectorphp/rector#1453

Closed

carusogabriel mentioned this pull request May 29, 2019

Add return type for bcmath extension functions #4201

Closed

BCmath fix for validation arguments #4092

BCmath fix for validation arguments #4092

Uh oh!

Conversation

vv12131415 commented Apr 30, 2019

Uh oh!

cmb69 commented Apr 30, 2019

Uh oh!

vv12131415 commented Apr 30, 2019

Uh oh!

vv12131415 commented Apr 30, 2019

Uh oh!

nikic commented May 1, 2019

Uh oh!

vv12131415 commented May 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic commented May 7, 2019

Uh oh!

vv12131415 commented May 11, 2019

Uh oh!

nikic May 13, 2019

Choose a reason for hiding this comment

Uh oh!

cmb69 May 13, 2019

Choose a reason for hiding this comment

Uh oh!

vv12131415 May 13, 2019

Choose a reason for hiding this comment

Uh oh!

cmb69 May 14, 2019

Choose a reason for hiding this comment

Uh oh!

vv12131415 May 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vv12131415 commented May 13, 2019

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

nikic commented May 14, 2019

Uh oh!

cmb69 commented May 14, 2019

Uh oh!

vv12131415 commented May 14, 2019

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic commented May 14, 2019

Uh oh!

Uh oh!