Skip to content

Fix stream double free in phar #18953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ext/phar/phar_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -2306,6 +2306,13 @@ static zend_object *phar_convert_to_other(phar_archive_data *source, int convert
/* exception already thrown */
return NULL;
}

if (newentry.fp == NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like the wrong solution. You're resetting the file pointer but phar_copy_file_contents already used it.
I believe the right solution is to:

  • Reset fp and cfp right after the copy from entry to newentry. The new file pointer will then be opened and set correctly in phar_copy_file_contents
  • Remove the /* save for potential restore on error */ block that moves the file pointers as this recovery mechanism isn't implemented as far as I see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard for me to understand why phar codes use cfp, so I use this strange solution to avoid break other features.

Also, there are many memcpys (like newentry <- entry here), dont know why, so I chose to not modify phar_copy_file_contents

/* entry->fp may be moved to cfp in phar_copy_file_contents
* and be free'd later in phar_flush_ex
* If so, zero fp to avoid double free in rshutdown. */
entry->fp = NULL;
}
no_copy:
newentry.filename = zend_string_copy(newentry.filename);

Expand Down
45 changes: 45 additions & 0 deletions ext/phar/tests/gh18953.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--TEST--
Phar: Stream double free
--EXTENSIONS--
phar
--INI--
phar.readonly=0
--ENV--
USE_ZEND_ALLOC=0
Copy link
Member

Choose a reason for hiding this comment

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

Drop this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phar.readonly=0 is needed, as for USE_ZEND_ALLOC=0, it helps reproducing in common php test environment (without ASAN)

--FILE--
<?php

declare(strict_types=1);

require __DIR__ . '/gh18953/autoload.inc';
Copy link
Member

Choose a reason for hiding this comment

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

You don't need all this autoloading stuff, nor the classes at the bottom. Executing the test without these things, with ASAN+USE_ZEND_ALLOC=0, reliably reproduces the bug.


// cleaning
@unlink("gh18953.phar");
@unlink("gh18953.phar.gz");
Comment on lines +16 to +18
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed because of the CLEAN section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If last test failed without cleaning, this test will never success. So I unlink them here.

Copy link
Member

Choose a reason for hiding this comment

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

A CLEAN section always runs, if it doesn't there are bigger problems with the test runner. Please remove it as asked previously.


// create a phar
$phar = new Phar("gh18953.phar");
$phar->startBuffering();
// add any dir
$phar->addEmptyDir("dir");
$phar->stopBuffering();
// compress
$phar->compress(Phar::GZ);

// this increases the chance of reproducing the problem
// even with this, it's not always reproducible
$obj1 = new NS1\Class1();
$obj2 = new NS1\Class1();
$obj2 = new NS1\Class1();
$obj2 = new NS1\Class1();
$obj2 = new NS1\Class1();

echo "Done" . PHP_EOL;
?>
--CLEAN--
<?php
@unlink("gh18953.phar");
@unlink("gh18953.phar.gz");
?>
--EXPECT--
Done
10 changes: 10 additions & 0 deletions ext/phar/tests/gh18953/autoload.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

spl_autoload_register(function ($class) {
$base_dir = __DIR__ . '/src/';

$file = $base_dir . str_replace('\\', '/', $class) . '.inc';
if (file_exists($file)) {
require $file;
}
});
Comment on lines +3 to +10
Copy link
Member

Choose a reason for hiding this comment

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

Can you not move this into the base test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure require matters, so I splited them

11 changes: 11 additions & 0 deletions ext/phar/tests/gh18953/src/NS1/Class1.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

namespace NS1;

use NS2\Interface1;

class Class1 implements Interface1
{
}
9 changes: 9 additions & 0 deletions ext/phar/tests/gh18953/src/NS2/Interface1.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace NS2;

interface Interface1
{
}