Cleanup redundant lookups in phar_object.c #10150
Conversation
| if (zend_hash_str_exists(&phar_obj->archive->manifest, newfile, (uint32_t) newfile_len)) { | ||
| if (NULL != (temp = zend_hash_str_find_ptr(&phar_obj->archive->manifest, newfile, (uint32_t) newfile_len)) || !temp->is_deleted) { | ||
| zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0, | ||
| "file \"%s\" cannot be copied to file \"%s\", file must not already exist in phar %s", oldfile, newfile, phar_obj->archive->fname); | ||
| RETURN_THROWS(); | ||
| } | ||
| if (NULL != (temp = zend_hash_str_find_ptr(&phar_obj->archive->manifest, newfile, (uint32_t) newfile_len)) && !temp->is_deleted) { |
There was a problem hiding this comment.
This is a behavioral change. It looks like a legit bugfix, so probably should be backported to PHP-8.1. And it would be a good idea to add a regression test, if feasible.
There was a problem hiding this comment.
Hi Christoph. While there is indeed a mistake in the original code that I fixed here, the bug here is not actually triggerable because of the zend_hash_str_exists check above that (indirectly) ensures that temp can never be NULL. The NULL check is eliminated by the compiler because temp->is_deleted would dereference a NULL pointer, which is UB. So the compiler optimizes this if to an assignment and a check if !temp->is_deleted. Combined with the zend_hash_str_exists above, this means that the old code has the exact same behaviour as the new code. But the new code does it in a imo clearer way.
So I don't think it's necessary to backport this change.
(In fact, the buggy-looking code was the reason I made this PR)
There was a problem hiding this comment.
Isn't the old behavior: if newfile exists in the .manifest, throw an exception, but the new behavior: if newfile exists in the .manifest, only throw an exception if !.is_deleted? In other words, the old code completely ignored the .is_deleted flag. Or am I missing something?
There was a problem hiding this comment.
It's a bit tricky. I now think you're right that this might need backporting because it depends on what the compiler decides to do.
The behaviour you describe would be the case if the original code did not have undefined behaviour. But because of the undefined behaviour, the NULL check is optimized away on my compiler, and it "correctly" checks for the is_deleted flag.
However, this depends on the compiler and its version and probably the optimisation level, so it might very well be that your compiler does something different which results in incorrect behaviour...
I tried writing a testcase for this but couldn't trigger the bug with my compiler.
There was a problem hiding this comment.
Yeah, I wouldn't rely on any particular compiler wrt. UB. Anyhow, I'll have a closer look soonish.
A minor code cleanup:
zend_hash_str_find_ptrwill only return an entry if it exists, so thezend_hash_str_existscall is redundant.