Remove broken check in var_unserializer #13852
Conversation
`end = *p+maxlen`, and pointer overflow is UB, so that means that a check of the form `end < *p` will always be false because it can only be true on pointer overflow. In particular, the compiler simplifies this to `maxlen < 0` which is always false because maxlen is unsigned.
|
Somewhat related: #10214 which previously removed a pointer overflow in the unserializer. |
| if (end < *p) { | ||
| zend_string_efree(str); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
This may not break anything that wasn't already broken because this was optimized out by the compiler, but I would prefer if we fixed this check instead of simply removing it
There was a problem hiding this comment.
The intention of the check seems to be to see if maxlen wraps around the address space.
I don't think that's possible, given that maxlen = max - YYCURSOR; and the fact that max is the pointer to the end of the input string.
So I'm not sure what you want me to replace this check with?
There was a problem hiding this comment.
You are right. I was blindly assuming that maxlen was possibly user-controlled. Looking at the history this check made more sense at some point, but now it can be removed.
There was a problem hiding this comment.
This is out of scope, but we could pass max directly to this function, maybe via UNSERIALIZE_PASSTHRU/UNSERIALIZE_PARAMETER.
| if (end < *p) { | ||
| zend_string_efree(str); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
You are right. I was blindly assuming that maxlen was possibly user-controlled. Looking at the history this check made more sense at some point, but now it can be removed.
end = *p+maxlen, and pointer overflow is UB, so that means that a check of the formend < *pwill always be false because it can only be true on pointer overflow. In particular, the compiler simplifies this tomaxlen < 0which is always false because maxlen is unsigned.