ext/bcmath: Added fuzzer for divide #18045
Conversation
| zend_long scale = char_to_size_t(scale_str); | ||
| free(scale_str); | ||
|
|
||
| if (fuzzer_request_startup() == FAILURE) { |
There was a problem hiding this comment.
This should happen earlier to not leak memory.
There was a problem hiding this comment.
Removed unnecessary memory allocations and relocated code.
How about the code now?
e1fa2fa
sapi/fuzzer/fuzzer-bcmath.c
Outdated
| ZVAL_STRINGL(&args[2], divisor_str, divisor_len); | ||
| ZVAL_LONG(&args[3], scale); | ||
|
|
||
| fuzzer_call_php_func_zval("bcdiv", 4, args); |
There was a problem hiding this comment.
It would be great to extend the fuzzer to pick one of a few bc function at random (e.g. bcadd, bcsub, ...)
There was a problem hiding this comment.
Changed in bd630e3
sapi/fuzzer/fuzzer-bcmath.c
Outdated
| ZVAL_UNDEF(&result); | ||
|
|
||
| zval args[4]; | ||
| ZVAL_COPY_VALUE(&args[0], &result); |
There was a problem hiding this comment.
Very confused by this line, fuzzer_call_php_func_zval uses a local zval for the result. You only need to pass 3 arguments as far as I can tell.
sapi/fuzzer/fuzzer-bcmath.c
Outdated
|
|
||
| int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) { | ||
| /* num1,num2,scale */ | ||
| const uint8_t *Comma1 = memchr(Data, ',', Size); |
There was a problem hiding this comment.
Please just use snake_case instead of CamelCase for variables
sapi/fuzzer/fuzzer-bcmath.c
Outdated
|
|
||
| zend_long char_to_size_t(char *c) { | ||
| zend_long ret = 0; | ||
| if (*c >= '0' && *c <= '9') { |
There was a problem hiding this comment.
This could also cause overflow
There was a problem hiding this comment.
Fixed in 82ed67d
Parsing scale was left unfinished code...
sapi/fuzzer/fuzzer-bcmath.c
Outdated
| return len; | ||
| } | ||
|
|
||
| size_t LLVMFuzzerCustomMutator(uint8_t *Data, size_t Size, size_t MaxSize, unsigned int Seed) { |
There was a problem hiding this comment.
I'm not so sure a custom mutator is the best approach. You're also not really mutating the buffer here.
There was a problem hiding this comment.
Wouldn't it be better to let the fuzzer mutate the input but just choose to interpret the raw bytes as numbers?
There was a problem hiding this comment.
Thanks, does that mean accepting the mutated input as is? Or does it mean, for example, taking it, chopping it into 64-bit chunks, interpreting them as integers, converting them to numeric characters, and concatenating them?
I had trouble creating numeric data with no decimal points or just one, so I used a custom mutator.
|
When building with the fuzzer enabled, it was quite troublesome if the generated executable file was under git management, so I listed it in |
|
I forgot to fix the README. There are no more additional fixes to be made. |
| sprintf(func_name, "%s", "bcmod"); | ||
| break; | ||
| case 5: | ||
| sprintf(func_name, "%s", "bcpow"); |
There was a problem hiding this comment.
When the exponent is large, the processing is too slow. It may be better to set a limit such as "when num2 is less than 1000"...
Since the BCMath code has changed significantly since version 8.4, I thought that if I were to introduce a fuzzer, it would be best to version 8.4 or later.
This is my first time working with fuzzers, so I don't know who to ask for a review...