fix bug #71317 by checking for duplicate filenames and using the last… #1748
Conversation
|
This has been introduced by 89beb12 as a fix for https://bugs.php.net/bug.php?id=68809. The PHP 5.6 implementation, if I'm reading the code correctly, used the first occurrence of the file, rather than the last one. We could either go back to that, use the last occurrence (what you implemented, consistent with tar) or keep the error (and change the message to actually tell you why it failed). The duplicate files in the PEAR packages, are they just the exact same file occurring multiple times (same content)? I'm a bit skeptical about removing the check as this seems like a packaging bug that might be better fixed on PEARs side. |
| newentry = zend_hash_str_update_mem(&myphar->manifest, entry.filename, entry.filename_len, (void*)&entry, sizeof(phar_entry_info)); | ||
| } else { | ||
| newentry = zend_hash_str_add_mem(&myphar->manifest, entry.filename, entry.filename_len, (void*)&entry, sizeof(phar_entry_info)); | ||
| } |
There was a problem hiding this comment.
It should be sufficient to use only an zend_hash_str_update_mem call here. In that case you should also replace the newentry == NULL check below with a ZEND_ASSERT(newentry != NULL), as it can no longer occur.
|
That commit actually showed the cause of the error, however it does not fix the actual problem. The 5.6 implementation actually causes a memory leak as I described in a bugreport I filed this morning I agree that i'ts a packaging bug on the PEAR end, however i feel that the PHP implementation should be able to gracefully handle this situation, in the same manor that tar handles the problem. I'm not well versed enough in the internals, but I will try your suggestion, for only using zend_hash_str_update_mem |
…ng that newentry != NULL
|
I've updated the fix to reflect your suggestion and it's way more elegant this way. If I understand correctly zend_hash_str_update_mem internally does a _zend_hash_str_add_or_update, always overwriting previous entries, in keeping with the sandard tar behaviour I think this is what we should aim for. |
|
The fix itself looks fine to me, I wonder if it's possible to improve the test. I am not familiar at all with the phar API, is it possible to demonstrate which of two files with the same name is chosen? I.e. show that it's the second one rather than the first (as opposed to PHP 5 behavior)? |
|
Is there a timeline when this patch will get upstream? Composer isn't working because of this, which is quite a big problem for some folks. Thanks! |
…cted instead of the first file, in line the behaviour of tar for amended files
|
I've created an extra test showing that when a file with the same name gets amended to the tar file the amended file gets extracted, this is the same behavior that as manually extracting the same file by running "tar -xf filename" on the command line. I've added the same test to the pr for 5.6 |
bug #71317 is caused by tar files having duplicate filenames.
Apparently several PEAR packages seem to suffer from this problem making it imposible to install them using composer when running php 7 since is uses Phardata->extractTo to extract the files
HTML_CSS-1.5.4.tgz is an example of this as a testfile for the testcase, bug71317.phpt.
Tar by default overwrites duplicate files, so I tried to mirror this behaviour when parsing the tar file by checking if the filename exists in the hash table, and if it does updating it with the entry of the newly found entry with the same name.
I'm not a C programmer so this will probably need some review.