[2015-07-08 14:30 UTC] ab@php.net

 [2015-07-17 15:13 UTC] ab@php.net

 [2015-07-17 15:13 UTC] ab@php.net

ACK. Sounds plausible to me, we should sync with the ext/zip behavior. Even if we break the spec in this case, it's a more secure way for specific PHP tasks.

Thanks.

 [2015-08-02 12:59 UTC] ab@php.net

I've checked the patch with master. There are a few issues.

- ZMM routines should be used
- for the Windows part, it has to retry (or maybe even try as first) with the backslash
- with the two above solved - the contents of the file is completely broken. While ext/zip extracted file contains "test", the ext/phar extracted file contains just garbage

Cheers.

 [2015-08-02 13:02 UTC] ab@php.net

But the contents of the file part seems to be an unrelated issue, what I had to mention.

Thanks.

 [2015-08-02 17:26 UTC] stas@php.net

Anatol, I'm not sure I fully understand the comments.

> - ZMM routines should be used

Where exactly?

> - for the Windows part, it has to retry (or maybe even try as first) with the backslash

Why? Doesn't Windows accept backslash as path separator? Furthermore, if it started with \, wouldn't path like \Foo be intereted as \\foo, i.e. UNC path? That's not what we want.

> - with the two above solved - the contents of the file is completely broken. While ext/zip extracted file contains "test", the ext/phar extracted file contains just garbage

I don't think this is caused by the patch, unless I miss something. Does it work for other ZIP files without the patch?

 [2015-08-02 20:19 UTC] ab@php.net

Hi Stas,

ZMM routines - yep, looks like all. I see now - it has changed in 5.6 when the virtual cwd was moved into Zend (just forgot it as it was a patch i took over). 5.5 inclusive uses plain CRT routines.

With the backslash - ok, see now it is the difference between ext/phar and ext/zip. The latter accepts both, maybe it lays on libzip, but i havent checked. Just as fact - the exctraction doesn't work when only '/' is checked on Windows. But it is also the part about the actual streams feature in handling paths. Fe ext/zip checks for IS_SLASH https://github.com/php/php-src/blob/master/ext/zip/php_zip.c#L96 . Maybe it's just simpler to check for both back and forward slash to achieve the behavior identical to ext/zip.

With the file contents - it is strange. I've created one archive with file ../hello.txt containing the string "hello". Then i've checked with and without the patch. Linux - both with and without patch file has broken content. Windows - broken file contents with the patch, correct contents without the patch. Seems the patch somehow affects this, but seems the main breakage is not caused by the patch itself, anyway.

Regards

Anatol

 [2015-08-02 20:52 UTC] stas@php.net

I'm not sure what you're proposing with regard to slashes on Windows. Current patch does not "check" anything, it just expands the path, eliminating the .. and . elements. Doesn't it work on Windows? What is the result? Sorry, I don't have windows setup handy, so I can't check. Also can't check the broken extraction - on my machine, it's broken before and after patch.

 [2015-08-02 22:26 UTC] ab@php.net

Yep, in the current state, per test, it'll fail to unzip on winodws. What I refer to is that new_state.cwd[0] = '\\'; should be checked as well - either as a retry or prior to '/'. It only regards to the relative path evaluation and cleanup. Merely what is being evaluated is the pure path before it even gets extracted, so on windows both '\\' and '/' are valid.

The file contents is a different issue, as you point out, too. Good that there's one more confirmation for that.

Thanks.

 [2015-08-02 23:32 UTC] stas@php.net

I'm not sure I understand - if both / amd \ are valid, why code with / failing right now on Windows? What about UNC paths - if we just try it with \ on Windows, it would interpret a path beginning with \\ as UNC path, not?

 [2015-08-03 10:36 UTC] ab@php.net

Hi Stas,

here's a modified patch https://gist.github.com/weltling/718ef2c5a1d9062e4e3a , using the DEFAULT_SLASH approach and one more improvement for long file names tested in ext\phar\tests\*extract*.phpt. Applies to 5.4 and 5.5. For 5.6 and 7.0 it seems to apply and work as well, just need to replace malloc/free with emalloc/efree before.

Thanks.

 [2015-08-04 22:22 UTC] stas@php.net

 [2015-09-09 10:01 UTC] kaplan@php.net

 [2016-03-23 13:08 UTC] mustafa at yavuzm dot com

I had same problem. When I extraxt on desktop files are working normally but when I extract zip file with phar command (extractTo) although all files are extracted they are broken o not working.