Skip to content

made scale argument of bcmath functions non-negative integer - #5455

Closed
vladysta wants to merge 1 commit into
php:masterfrom
vladysta:strict-nonzero-scale-bcmath-fns
Closed

made scale argument of bcmath functions non-negative integer#5455
vladysta wants to merge 1 commit into
php:masterfrom
vladysta:strict-nonzero-scale-bcmath-fns

Conversation

@vladysta

Copy link
Copy Markdown
Contributor

If you think I should make RFC, I will. From my POV it's just bugs and next (8) release can break things a little bit, so I'm making it more strict.

The only thing that bothers me is ext/bcmath/tests/bug60377.phpt. I did print out the

<?phpechogettype(634314234334311);

but it says integer. But internally it, somehow breaks and makes scale 0, this haven't worked before (I mean I haven't break the behavior where 634314234334311 casts to 0) I have tried to do this before patch

<?phpbcscale(634314234334311);
echobcscale();

and got 0.

I may try to fix it, but I think

  1. it's other PR
  2. some C lang expert (not me) would make it faster

@vladysta
vladystaforce-pushed the strict-nonzero-scale-bcmath-fns branch from ebf7e7c to 6dcbac8CompareApril 24, 2020 23:45
Comment threadext/bcmath/bcmath.c Outdated
@vladysta
vladystaforce-pushed the strict-nonzero-scale-bcmath-fns branch from 6dcbac8 to 5fddc27CompareApril 25, 2020 15:37
@vladysta
vladysta requested a review from cmb69April 25, 2020 15:56
@vladysta

Copy link
Copy Markdown
ContributorAuthor

Can't add reviewers, so I'm just adding you to this message @nikic@kocsismate

Comment threadext/bcmath/bcmath.c Outdated

@cmb69cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Please address my inline comments.

Also, I think it is worthwhile to change bc_precision to int as well, and to introduce a custom validator, something like:

 ext/bcmath/bcmath.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/ext/bcmath/bcmath.c b/ext/bcmath/bcmath.c
index 38fa4d033d..35d1bc92f6 100644
--- a/ext/bcmath/bcmath.c+++ b/ext/bcmath/bcmath.c@@ -57,9 +57,32 @@ ZEND_TSRMLS_CACHE_DEFINE()
ZEND_GET_MODULE(bcmath)
#endif
+ZEND_INI_MH(OnUpdateScale)+{+	int *p;+	zend_long tmp;+#ifndef ZTS+	char *base = (char *) mh_arg2;+#else+	char *base;++	base = (char *) ts_resource(*((int *) mh_arg2));+#endif++	tmp = zend_atol(ZSTR_VAL(new_value), ZSTR_LEN(new_value));+	if (tmp < 0 || tmp > INT_MAX) {+ return FAILURE;+	}++	p = (int *) (base+(size_t) mh_arg1);+	*p = (int) tmp;++	return SUCCESS;+}+
/* {{{ PHP_INI */
PHP_INI_BEGIN()
-	STD_PHP_INI_ENTRY("bcmath.scale", "0", PHP_INI_ALL, OnUpdateLongGEZero, bc_precision, zend_bcmath_globals, bcmath_globals)+	STD_PHP_INI_ENTRY("bcmath.scale", "0", PHP_INI_ALL, OnUpdateScale, bc_precision, zend_bcmath_globals, bcmath_globals)
PHP_INI_END()
/* }}} */

Comment threadext/bcmath/bcmath.c Outdated
Comment threadext/bcmath/tests/negative_scale.phpt Outdated
@vladysta
vladystaforce-pushed the strict-nonzero-scale-bcmath-fns branch from a20c9bd to 263d335CompareApril 26, 2020 15:05
@vladysta

Copy link
Copy Markdown
ContributorAuthor

Now the scale INI is not set. https://travis-ci.org/github/php/php-src/jobs/679741601#L2251

Can you help, to say what is wrong?

@cmb69

Copy link
Copy Markdown
Member

Oh, sorry, my patch was incomplete. Need this as well:

 ext/bcmath/php_bcmath.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ext/bcmath/php_bcmath.h b/ext/bcmath/php_bcmath.h
index bafaf29e7a..c7a8a85d73 100644
--- a/ext/bcmath/php_bcmath.h+++ b/ext/bcmath/php_bcmath.h@@ -33,7 +33,7 @@ ZEND_BEGIN_MODULE_GLOBALS(bcmath)
bc_num _zero_;
bc_num _one_;
bc_num _two_;
-	zend_long bc_precision;+	int bc_precision;
ZEND_END_MODULE_GLOBALS(bcmath)
#if defined(ZTS) && defined(COMPILE_DL_BCMATH)

@cmb69cmb69 closed this Apr 26, 2020
@cmb69cmb69 reopened this Apr 26, 2020
@vladysta
vladystaforce-pushed the strict-nonzero-scale-bcmath-fns branch from 263d335 to 0022353CompareApril 26, 2020 17:44

@cmb69cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

Comment threadext/bcmath/bcmath.c Outdated
Comment threadext/bcmath/bcmath.c Outdated
 - code styles (spaces -> tabs)
- add custom ini setting validator (OnUpdateScale)
@vladysta
vladystaforce-pushed the strict-nonzero-scale-bcmath-fns branch from 0022353 to c167235CompareApril 27, 2020 09:32
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@vladysta@cmb69@nikic@kocsismate