Skip to content

Commit

Permalink
Explicitly validate popen mode
Browse files Browse the repository at this point in the history
To avoid behavior differences due to libc.
  • Loading branch information
nikic committed Aug 6, 2020
1 parent 0dda242 commit ab36540
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
8 changes: 8 additions & 0 deletions ext/standard/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -930,10 +930,18 @@ PHP_FUNCTION(popen)
char *z = memchr(posix_mode, 'b', mode_len);
if (z) {
memmove(z, z + 1, mode_len - (z - posix_mode));
mode_len--;
}
}
#endif

/* Musl only partially validates the mode. Manually check it to ensure consistent behavior. */
if (mode_len != 1 || (*posix_mode != 'r' && *posix_mode != 'w')) {
php_error_docref2(NULL, command, posix_mode, E_WARNING, "Invalid mode");
efree(posix_mode);
RETURN_FALSE;
}

fp = VCWD_POPEN(command, posix_mode);
if (!fp) {
php_error_docref2(NULL, command, posix_mode, E_WARNING, "%s", strerror(errno));
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/tests/file/popen_pclose_error.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ unlink($file_path."/popen.tmp");
--EXPECTF--
*** Testing for error conditions ***

Warning: popen(abc.txt,rw): %s on line %d
Warning: popen(abc.txt,rw): Invalid mode in %s on line %d
bool(false)

--- Done ---

2 comments on commit ab36540

@cmb69
Copy link
Contributor

@cmb69 cmb69 commented on ab36540 Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic, that doesn't work for Windows where 't'/'b' are still a thing (I hope to come to that soon) causing ext\standard\tests\streams\stream_get_meta_data_process_basic.phpt to fail. I don't think that check is needed for Windows, so the new code could go into the #ifndef PHP_WIN32 above.

@nikic
Copy link
Member Author

@nikic nikic commented on ab36540 Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 Thanks for the notice. You are right that the check should be inside the ifndef PHP_WIN32. I've reverted the change for now.

Please sign in to comment.