Repository navigation
Conversation
The merged value was not marked persistent local, which tripped the ZEND_RC_DEBUG assertions, and the second value leaked. Spellings differing only by case also left headers_original_case pointing at the value freed by the headers update. Repeats now update the entry of the first spelling.
| } | ||
|
|
||
| zend_string *orig = client->current_header_name; | ||
| if (entry != NULL) { |
There was a problem hiding this comment.
I don't understand this and the PR doesn't explain why this is needed in simple English
Presumably, one of the hash updates free a string, but there must be a cleaner and simpler solution than looping over the entire hash table, right?
There was a problem hiding this comment.
alright I ll think of something better
There was a problem hiding this comment.
Either you can get the string from the bucket itself; or you stop borrowing the pointer, give headers_original_case its destructor, and use ADDREF
There was a problem hiding this comment.
I went for the 2nd solution already locally but will commit soon.
| zend_hash_update(&client->request.headers, lc_header_name, &tmp); | ||
| /* A repeat spelled differently leaves the first spelling's entry in place while the | ||
| * update above releases its value, so this table must own a reference too. */ | ||
| zend_string_addref(Z_STR(tmp)); |
There was a problem hiding this comment.
the update above releases its value
Then why does the addref come after the update?
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Seems okay now.
The array shows pre-existing weird behaviour in that both the stale and the new header values are shown, while we'd expect only the merged ones to remain intuitively, but that's master-only material I believe...
The merged value was not marked persistent local, which tripped the ZEND_RC_DEBUG assertions, and the second value leaked. Spellings differing only by case also left headers_original_case pointing at the value freed by the headers update. Repeats now update the entry of the first spelling.