Repository navigation
Conversation
cmb69
left a comment
cmb69
left a comment
There was a problem hiding this comment.
I think this is good as is, but see my comments below for an alternative solution, which appears to be cleaner, and which wouldn't have crashed in the first place.
There was a problem hiding this comment.
I cannot easily test this (non Windows extension), but I figure that even mesage_len == 0 wouldn't be a problem, if we didn't access msg_var.s directly, but rather via smart_str APIs only. See comment below.
There was a problem hiding this comment.
I think this shouldcould probably be something like (untested!):
| memcpy(messagebuffer->mtext, ZSTR_VAL(msg_var.s), message_len + 1); | |
| smart_str_free(&msg_var); | |
| zend_string *str = smart_str_extract(msg_var); | |
| memcpy(messagebuffer->mtext, ZSTR_VAL(str), message_len + 1); | |
| zend_string_release(str); |
There was a problem hiding this comment.
Did not test it yet ; will do in the next hour, but that should work too, I just wanted to have an early exit/not bothering creating an empty message.
There was a problem hiding this comment.
I just wanted to have an early exit/not bothering creating an empty message.
Makes sense, but still could use proper smart_str APIs instead of accessing the underlying zend_string directly.
|
While this fix is good, it's not a full fix. Additionally, if serializing resulted in an exception we shouldn't even try to send a message in the first place. |
sure, will give a try. |
There was a problem hiding this comment.
If we use smart_str_extract(), I'd probably avoid getting the message_len from the smart_str, i.e.
zend_string *str = smart_str_extract(&msg_var);
messagebuffer = safe_emalloc(ZSTR_LEN(str), 1, sizeof(struct php_msgbuf));
memcpy(messagebuffer->mtext, ZSTR_VAL(str), ZSTR_LEN(str) + 1);But it's okay for me as is.
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
2 minor nits, but looks right otherwise
…s expected. It is assumed that the serialization always had initialised its buffer zend_string, but in the case of a type not serialising, it is null.
…xpected.
It is assumed that the serialization always had initialised its buffer zend_string, but in the case of a type not serialising, it is null.