Curl valgrind issue #318
Curl valgrind issue #318
Conversation
…urce when getting the handle ID to access the metadata hashmap
| } | ||
|
|
||
| #define ensure_curl_multi_metadata_hashmap() \ |
There was a problem hiding this comment.
I couldn't find a reason why several of these macros were used instead of functions. For easier readability- as well as less code duplication when adding the ifdefs to fix the issue- I changed them to functions. I also moved some functions around so that definitions were in the correct order.
agent/php_zval.h
Outdated
| if (!nr_php_is_zval_valid_resource(zv)) | ||
| { | ||
| return 0; | ||
| } | ||
| #ifdef PHP7 |
There was a problem hiding this comment.
Can you please update the style for readability:
#if ZEND_MODULE_API_NO >= ZEND_7_0_X_API_NO /* PHP 7.0+ */
Currently PHP7 actually indicates PHP7+.
agent/php_curl_md.c
Outdated
| nr_php_curl_multi_md_t* multi_metadata; | ||
| size_t async_index; |
There was a problem hiding this comment.
Now that the id check is happening here, can we move the variable definitions up to the top of the function?
agent/php_curl_md.c
Outdated
| #else | ||
| uint64_t id = (uint64_t)nr_php_zval_resource_id(ch); | ||
| #endif | ||
| if (id == 0) |
There was a problem hiding this comment.
Flip around for yoda conditions (styleguide).
Same comment for here.
|
Aside from a few nits, it looks great! Thanks for finding/fixing this. It'll be great to get this fix merged. |
|
Looks like jenkins is showing some errors for the PHP 5.5 build which is probably why they were initially macros, but it's still fixable for the functions. I think you can just take the TSRM value you get here and pass it into your new functions that have TSRM in the signatures. |
This PR addresses an issue that was left unnoticed from the transition to PHP 8.0. There was still an instance of using the curl handle as a resource instead of an object. This issue was found when exploring the upgrade to PHP 8.1.