Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined float conversion in mb_convert_variables #17503

Closed
YuanchengJiang opened this issue Jan 18, 2025 · 6 comments
Closed

Undefined float conversion in mb_convert_variables #17503

YuanchengJiang opened this issue Jan 18, 2025 · 6 comments

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
xml_parse_into_struct(xml_parser_create_ns(), str_repeat("<blah>", 1000), $a);
$fusion = $a;
var_dump(mb_convert_variables("ASCII", ["UTF-8", "UTF-16"], $fusion));

Resulted in this output:

/home/phpfuzz/WorkSpace/flowfusion/php-src/ext/mbstring/mbstring.c:3351:23: runtime error: 1.91594e+19 is outside the range of representable values of type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/phpfuzz/WorkSpace/flowfusion/php-src/ext/mbstring/mbstring.c:3351:23

PHP Version

nightly

Operating System

No response

@devnexen
Copy link
Member

can't reproduce. First XML_MAXLEVEL prevents it from the get go. Even if I update it I still do not get the overflow in mbstring. But I guess it does not kill to put a check.

@youkidearitai
Copy link
Contributor

I can reproduce. seems curious.

$ sapi/cli/php -dmemory_limit=-1 gh17503.php
/home/tekimen/src/php-src/Zend/zend_ini_parser.y:377:4: runtime error: call to function php_ini_parser_cb through pointer to incorrect function type 'void (*)(struct _zval_struct *, struct _zval_struct *, struct _zval_struct *, int, void *)'
/home/tekimen/src/php-src/main/php_ini.c:184: note: php_ini_parser_cb defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/Zend/zend_ini_parser.y:377:4
/home/tekimen/src/php-src/Zend/zend_API.c:2424:4: runtime error: call to function zm_globals_ctor_date through pointer to incorrect function type 'void (*)(void *)'
/home/tekimen/src/php-src/ext/date/php_date.c:395: note: zm_globals_ctor_date defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/Zend/zend_API.c:2424:4
/home/tekimen/src/php-src/Zend/zend_variables.c:57:2: runtime error: call to function zend_string_destroy through pointer to incorrect function type 'void (*)(struct _zend_refcounted *)'
/home/tekimen/src/php-src/Zend/zend_variables.c:62: note: zend_string_destroy defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/Zend/zend_variables.c:57:2

Warning: xml_parse_into_struct(): Maximum depth exceeded - Results truncated in /home/tekimen/src/php-src/gh17503.php on line 3
/home/tekimen/src/php-src/ext/mbstring/mbstring.c:3351:23: runtime error: 1.91594e+19 is outside the range of representable values of type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/ext/mbstring/mbstring.c:3351:23
string(5) "UTF-8"
/home/tekimen/src/php-src/Zend/zend_llist.c:93:7: runtime error: call to function zend_compare_file_handles through pointer to incorrect function type 'int (*)(void *, void *)'
/home/tekimen/src/php-src/Zend/zend_stream.c:251: note: zend_compare_file_handles defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/Zend/zend_llist.c:93:7
/home/tekimen/src/php-src/Zend/zend_llist.c:94:4: runtime error: call to function zend_file_handle_dtor through pointer to incorrect function type 'void (*)(void *)'
/home/tekimen/src/php-src/Zend/zend_stream.c:214: note: zend_file_handle_dtor defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/Zend/zend_llist.c:94:4
/home/tekimen/src/php-src/Zend/zend_API.c:3341:4: runtime error: call to function zm_globals_dtor_phar through pointer to incorrect function type 'void (*)(void *)'
/home/tekimen/src/php-src/ext/phar/phar.c:3410: note: zm_globals_dtor_phar defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tekimen/src/php-src/Zend/zend_API.c:3341:4

@devnexen
Copy link
Member

Ah right I can only reproduce with clang :) feel free to assign to yourself.

@cmb69
Copy link
Member

cmb69 commented Jan 19, 2025

array[i].demerits = (uint64_t) (array[i].demerits * array[i].multiplier);

I think we need to clamp the value instead of casting to uint64_t (might need the cast in addition, though). Possibly:

		array[i].demerits = MIN(array[i].demerits * array[i].multiplier, UINT64_MAX);

@youkidearitai, regarding the "call to function through pointer to incorrect function type" errors, see #17462. TL; DR: also build with -fno-sanitize=function.

@alexdowad
Copy link
Contributor

@cmb69 is right that merely casting, rather than clamping, will cause mb_detect_encoding to make very strange guesses of input text encoding.

@alexdowad
Copy link
Contributor

@cmb69 is right that merely casting, rather than clamping, will cause mb_detect_encoding to make very strange guesses of input text encoding.

...Which, by the way, is my fault. 😄 Thanks to @YuanchengJiang for catching my mistake.

cmb69 added a commit to cmb69/php-src that referenced this issue Feb 3, 2025
Conversion of floating point to integer values is undefined if the
integral part of the float value cannot be represented by the integer
type.  We need to cater to that explicitly (in a manner similar to
`zend_dval_to_lval_cap()`).
@cmb69 cmb69 changed the title UBSan mb_convert_variables() with long string Undefined float conversion in mb_convert_variables Feb 3, 2025
@cmb69 cmb69 closed this as completed in 55e676e Feb 4, 2025
cmb69 added a commit that referenced this issue Feb 4, 2025
* PHP-8.3:
  Fix GH-17503: Undefined float conversion in mb_convert_variables
cmb69 added a commit that referenced this issue Feb 4, 2025
* PHP-8.4:
  Fix GH-17503: Undefined float conversion in mb_convert_variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants