ext/session refactoring: INI settings #9322
Conversation
ext/session/session.c
Outdated
| return FAILURE; | ||
| } | ||
| /* Nul bytes are not allowed */ | ||
| if (ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) { |
There was a problem hiding this comment.
This check is highly non-obvious. I was about to suggest changing the second to ZSTR_LEN as well.
There was a problem hiding this comment.
| if (ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) { | |
| if (CHECK_NULL_PATH(ZSTR_VAL(new_value), ZSTR_LEN(new_value))) { |
Maybe it makes sense to introduce another macro, which expects a zend_string.
|
The first commit can likely be spun out into a separate PR, because that one is a bugfix based on my understanding, whereas the later commits are rather a feature / refactoring. |
04df5ff to
a5bfad6
Compare
I'm not sure starting to emit warnings in a patch release is that ideal. |
a5bfad6 to
7ab79e4
Compare
|
I might have a possible explanation as to why I'm getting the refcount issues, is that the |
7ab79e4 to
c0178cc
Compare
However, the code is somewhat disgusting as I'm hitting Zend/zend_types.h:1222: zend_gc_delref: Assertion (zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7) failed. Failures that I don't understand.
Also fixed a bug where nul bytes where not properly checked
Co-authored-by: Tim Düsterhus <[email protected]>
c0178cc to
f9e3e29
Compare
If you want, you can rework the PR, as I have no recollection of this any more. |
Came about because of #9304 where the standard API requires zend_strings.
I do really not understand why I was getting some GC refcount issues so it's been a bit bodged together.