Fix off-by-one bug when truncating tempnam prefix #11870
Conversation
The tempnam documentation currently states that "Only the first 63 characters of the prefix are used, the rest are ignored". However when the prefix is 64 characters-long, the current implementation fails to strip the last character, diverging from the documented behavior. This patch fixes the implementation so it matches the documented behavior for that specific case where the prefix is 64 characters long.
There was a problem hiding this comment.
The documentation should be fixed. Not the source code.
The source of truth is the code in php-src, the docs are meant to describe the behaviour of php-src not indicate how it should behave.
Indeed if this was the case dynamic properties for objects should have been removed with no RFC needed as this was undocumented until the deprecation came into effect.
|
Hi @Girgias, thanks for the review! Still, I believe this is indeed a off-by-one bug with tempnam: Check this out: $ php -r 'var_dump(tempnam(".", str_repeat("x", 63)));'
string(78) "/tmp/foo/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx9Ze3MZ"
$ php -r 'var_dump(tempnam(".", str_repeat("x", 64)));'
string(79) "/tmp/foo/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx7Bt2mI"
$ php -r 'var_dump(tempnam(".", str_repeat("x", 65)));'
string(78) "/tmp/foo/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxFN2yvX"We currently allow the maximum prefix We could go the other way around and allow the strlen to go all the way up to 64 (and update the docs), but this would also require a code change. FWIW, the bogus behavior was introduced in php 5.1.3 in 4ca3df5. Before that the value was copied from a char[64] array with strlcpy, i.e, the prefix would indeed be truncated after 63 characters. However, I could not find the historical reasons behind the length limitation there. Would you rather just document that "the prefix argument on tempnam length can be as large as 64 characters. If it is greater than that, then it will be truntcated to use only the first 63 characters"? |
Right... so it is more complicated than just a difference in documented behaviour. So it seems this need to be fixed as shown, and possibly look if it makes sense to drop the length limit requirement for master |
I could not find the history behind that length. The git history only goes so far. I wonder if that is/was related to how temp files are created in specific systems. Then, I preferred to not touch the length atm. |
| } | ||
| rmdir($file_path); | ||
|
|
||
| ?> |
There was a problem hiding this comment.
The test should have a --CLEAN-- section removing the folder and all files within it
| for ($i=0; $i<count($names_arr); $i++) { | ||
| echo "-- Iteration $i --\n"; | ||
| try { | ||
| $file_name = tempnam("$file_path", $names_arr[$i]); | ||
| } catch (Error $e) { | ||
| echo $e->getMessage(), "\n"; | ||
| continue; | ||
| } | ||
|
|
||
| $base_name = basename($file_name); | ||
| echo "File name is => "; | ||
| print($base_name); | ||
| echo "\n"; | ||
| echo "File name length is => "; | ||
| print(strlen($base_name)); | ||
| echo "\n"; | ||
|
|
||
| if (file_exists($file_name)) { | ||
| unlink($file_name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not just use a foreach loop?
There was a problem hiding this comment.
I was trying to keep consistency with the other tempnam test variations. I changed it to a foreach (maybe it improves readability).
There was a problem hiding this comment.
Those tests are probably very old and not written in the best way
Ensure the string is properly truncated when its length is greater than 63 characters.
athos-ribeiro
force-pushed
the
tempnam-off-by-one
branch
from
ed77df4
to
cf3a772
Compare
The tempnam documentation currently states that "Only the first 63 characters of the prefix are used, the rest are ignored". However when the prefix is 64 characters-long, the current implementation fails to strip the last character, diverging from the documented behavior. This patch fixes the implementation so it matches the documented behavior for that specific case where the prefix is 64 characters long. Closes phpGH-11870 Signed-off-by: George Peter Banyard <girgias@php.net>
The tempnam documentation currently states that "Only the first 63
characters of the prefix are used, the rest are ignored". However when
the prefix is 64 characters-long, the current implementation fails to
strip the last character, diverging from the documented behavior. This
patch fixes the implementation so it matches the documented behavior for
that specific case where the prefix is 64 characters long.