Refactor implementation of ldap_modify_batch() #16033
Conversation
Girgias
force-pushed
the
ldap-modify_batch-refactoring
branch
3 times, most recently
from
b0f639d to
46a0773
Compare
| /* make sure the DN contains no NUL bytes */ | ||
| if (zend_char_has_nul_byte(dn, dn_len)) { | ||
| zend_argument_value_error(2, "must not contain null bytes"); | ||
| RETURN_THROWS(); | ||
| } |
There was a problem hiding this comment.
I will do a follow-up to use the p ZPP modifier consistently for the DN parameter.
| zend_hash_internal_pointer_reset(Z_ARRVAL_P(mods)); | ||
| if (zend_hash_get_current_key_type(Z_ARRVAL_P(mods)) != HASH_KEY_IS_LONG) { | ||
| zend_argument_type_error(3, "must be integer-indexed"); | ||
| SEPARATE_ARRAY(modification_zv); |
There was a problem hiding this comment.
I am not sure why we would need to separate the array as we are just doing validation and reading from it.
There was a problem hiding this comment.
Because later on the code uses convert_to_* calls, but that means the zval may get transformed causing either corruption with opcache; or spooky unexpected changes at a distance.
There was a problem hiding this comment.
Ahhhh okay thank you for the explanation.
|
|
||
| zend_hash_move_forward(Z_ARRVAL_P(mod)); | ||
| } | ||
| SEPARATE_ARRAY(modification_values_zv); |
There was a problem hiding this comment.
Ditto, not sure why we would need this.
| zend_string *modval = zval_get_string(modification_value_zv); | ||
| if (EG(exception)) { | ||
| RETVAL_FALSE; | ||
| ldap_mods[i]->mod_bvalues[j] = NULL; | ||
| num_mods = i + 1; | ||
| ldap_mods[modification_index]->mod_bvalues[value_index] = NULL; | ||
| num_mods = modification_index + 1; | ||
| goto cleanup; | ||
| } |
There was a problem hiding this comment.
This case should probably be moved to the validation step ensuring that all values are strings.
Girgias
force-pushed
the
ldap-modify_batch-refactoring
branch
from
46a0773 to
feccb4c
Compare
There was a problem hiding this comment.
I found this hard to check because of the combination of indentation changes (which marks everything as changed) and semantic changes. I'd rather have a commit that changes the indentation and then the one after doing semantic changes. I realize it's not always possible to do this though.
Also a general thought: the arrays are validated but don't handle references, so you can get strange errors like "Option X must be of type Y, Y given"
| --FILE-- | ||
| <?php | ||
|
|
||
| /* We are assuming 3333 is not connectable */ |
There was a problem hiding this comment.
I'm wondering if there's a reserved port or something like that we can use. I can imagine this test could be a tad bit flaky on some systems depending on the services running, unsure.
There was a problem hiding this comment.
I picked this from another bug test, and it seems rather reliable
| zend_hash_internal_pointer_reset(Z_ARRVAL_P(mods)); | ||
| if (zend_hash_get_current_key_type(Z_ARRVAL_P(mods)) != HASH_KEY_IS_LONG) { | ||
| zend_argument_type_error(3, "must be integer-indexed"); | ||
| SEPARATE_ARRAY(modification_zv); |
There was a problem hiding this comment.
Because later on the code uses convert_to_* calls, but that means the zval may get transformed causing either corruption with opcache; or spooky unexpected changes at a distance.
|
|
||
| zend_hash_move_forward(Z_ARRVAL_P(mod)); | ||
| } | ||
| SEPARATE_ARRAY(modification_values_zv); |
|
Right references, I always forget about them. I will try to split the commit into different bits to make it more manageable. |
Girgias
force-pushed
the
ldap-modify_batch-refactoring
branch
from
feccb4c to
99982e5
Compare
And also make them throw ValueErrors instead of TypeErrors
Girgias
force-pushed
the
ldap-modify_batch-refactoring
branch
from
99982e5 to
19b2dfb
Compare
Rename some of the variables This also reduces the scope of some of these variables
Girgias
force-pushed
the
ldap-modify_batch-refactoring
branch
from
19b2dfb to
3759b81
Compare
Depends on #16032 getting fixed, as I found those issue while staring at the code and writing the tests.