Fix GH-16592 msg_send() crashes when the type does not serialize as e… #16599
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.
ext/sysvmsg/sysvmsg.c
Outdated
| if (!message_len) { | ||
| RETURN_FALSE; | ||
| } |
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.
ext/sysvmsg/sysvmsg.c
Outdated
| memcpy(messagebuffer->mtext, ZSTR_VAL(msg_var.s), message_len + 1); | ||
| smart_str_free(&msg_var); |
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. |
ext/sysvmsg/sysvmsg.c
Outdated
| message_len = smart_str_get_len(&msg_var); | ||
|
|
||
| /* NB: php_msgbuf is 1 char bigger than a long, so there is no need to | ||
| * allocate the extra byte. */ | ||
| messagebuffer = safe_emalloc(ZSTR_LEN(msg_var.s), 1, sizeof(struct php_msgbuf)); | ||
| memcpy(messagebuffer->mtext, ZSTR_VAL(msg_var.s), ZSTR_LEN(msg_var.s) + 1); | ||
| message_len = ZSTR_LEN(msg_var.s); | ||
| messagebuffer = safe_emalloc(message_len, 1, sizeof(struct php_msgbuf)); | ||
| zend_string *str = smart_str_extract(&msg_var); | ||
| memcpy(messagebuffer->mtext, ZSTR_VAL(str), message_len + 1); |
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.
3 participants
…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.