Plain stream wrapper: Add assertions to uphold assumptions and remove a dubious API usage #17322
Conversation
Girgias
changed the title
Girgias
force-pushed
the
stream-fopen-assumptions-check-and-refactoring
branch
from
def7561 to
30003f5
Compare
Girgias
changed the title
Girgias
force-pushed
the
stream-fopen-assumptions-check-and-refactoring
branch
from
30003f5 to
78d365c
Compare
main/streams/plain_wrapper.c
Outdated
| if (expand_filepath(filename, realpath) == NULL) { | ||
| return NULL; | ||
| } | ||
| realpath_len = strlen(realpath); |
There was a problem hiding this comment.
This might wasteful - it's not used in all paths below. Just keep strlen(realpath) - this function is not really used so not much point to change it.
main/streams/plain_wrapper.c
Outdated
| realpath_len = strlen(filename); | ||
| ZEND_ASSERT(realpath_len < sizeof(realpath) && "Assumed real path filename is too long"); | ||
| strcpy(realpath, filename); |
There was a problem hiding this comment.
This is an external API so there should be some prevention - you should at least use strncpy if you really need to replace this.
There was a problem hiding this comment.
you can keep the assert but not use only strcpy.
There was a problem hiding this comment.
strncpy does do padding, which is pointless. Actually, strlcpy here was fine IMO because with the new code we now do two traversals.
There was a problem hiding this comment.
yeah good point. Let's close this PR in that case...
There was a problem hiding this comment.
maybe just the top one makes sense. I don't really mind. This function is not even used from anywhere in the core - at least I didn't find any usage...
There was a problem hiding this comment.
there might still be external usage ofc. as it's public API.
There was a problem hiding this comment.
I dropped the first commit
The description of PHP_STRLCPY says that this is a fast version of strlcpy that should be used if we *know* the size of both the source and destination buffers. This is clearly not the case as we use strlen() to compute it. Moreover if the result cannot fit in the destination buffer something seriously strange has happened and we should return a failure state rather than truncating.
Girgias
force-pushed
the
stream-fopen-assumptions-check-and-refactoring
branch
from
78d365c to
80e6701
Compare
3 participants
No description provided.