Fixes to phar stub #10706

nielsdos · 2023-02-25T23:49:04Z

There were a couple of issues with the phar stub. It seems that in the past people did not realise they had to change shortarc.php and run makestub.php to generate the stubfile and instead they modified stub.h manually. This meant that there were a couple of mistakes in the stub which are fixed in this patch. In particular:

The title tag was not closed
The length of the stub was wrong in stub.h
This PR syncs previous changes to stub.h back to shortarc.php and vice versa.
Adds a note such that hopefully no mistakes against updating the stubs are made in the future (hopefully).
The makestub.php script was actually broken because the expected signature of the stub got changed in b874f1a

There were a couple of issues with the phar stub. It seems that in the past people did not realise they had to change shortarc.php and run makestub.php to generate the stubfile and instead they modified stub.h manually. This meant that there were a couple of mistakes in the stub which are fixed in this patch. In particular: * The title tag was not closed * The length of the stub was wrong in stub.h * This PR syncs previous changes to stub.h back to shortarc.php and vice versa. * Adds a note such that hopefully no mistakes against updating the stubs are made in the future (hopefully). * The makestub.php script was actually broken because the expected signature of the stub got changed in b874f1a

mvorisek · 2023-02-26T00:11:59Z

It seems that in the past people did not realise they had to change shortarc.php and run makestub.php to generate the stubfile and instead they modified stub.h manually.

assert that in https://github.com/php/php-src/blob/master/.github/actions/verify-generated-files/action.yml with CI

Girgias

This looks sensible but I don't know anything about phar tbh.

One question, is the change in file permission in makestub.php intended?

nielsdos · 2023-02-26T14:14:33Z

This looks sensible but I don't know anything about phar tbh.

I don't know a lot about phar too tbh, so best to let someone with more knowledge about phar double check I guess... 😅

One question, is the change in file permission in makestub.php intended?

Yes. This is needed so we can conveniently run it from the command line and from the CI script (see my second commit) :)

medabkari · 2024-01-14T18:46:46Z

I don't know a lot about phar too tbh, so best to let someone with more knowledge about phar double check I guess... 😅

This PR has been stale for almost a year. I remember Bishop was the maintainer of ext/phar, so if someone could ping him for a quick review, that would be helpful.

Girgias

This looks okay after a rebase to 8.2

Girgias · 2024-01-15T09:37:37Z

ext/phar/stub.h

+	static const char newstub3_1[] = "ction tmpdir()\n{\nif (strpos(PHP_OS, 'WIN') !== false) {\nif ($var = getenv('TMP') ? getenv('TMP') : getenv('TEMP')) {\nreturn $var;\n}\nif (is_dir('/temp') || mkdir('/temp')) {\nreturn realpath('/temp');\n}\nreturn false;\n}\nif ($var = getenv('TMPDIR')) {\nreturn $var;\n}\nreturn realpath('/tmp');\n}\n\nstatic function _unpack($m)\n{\n$info = unpack('V', substr($m, 0, 4));\n\n$l = unpack('V', substr($m, 10, 4));\n$m = substr($m, 14 + $l[1]);\n$s = unpack('V', substr($m, 0, 4));\n$o = 0;\n$start = 4 + $s[1];\n$ret['c'] = 0;\n\nfor ($i = 0; $i < $info[1]; $i++) {\n\n$len = unpack('V', substr($m, $start, 4));\n$start += 4;\n\n$savepath = substr($m, $start, $len[1]);\n$start += $len[1];\n\n\n\n$ret['m'][$savepath] = array_values(unpack('Va/Vb/Vc/Vd/Ve/Vf', substr($m, $start, 24)));\n$ret['m'][$savepath][3] = sprintf('%u', $ret['m'][$savepath][3]\n& 0xffffffff);\n$ret['m'][$savepath][7] = $o;\n$o += $ret['m'][$savepath][2];\n$start += 24 + $ret['m'][$savepath][5];\n$ret['c'] |= $ret['m'][$savepath][4] & self::MASK;\n}\nreturn $ret;\n}\n\nstatic function extractFile($path, $entry, $fp)\n{\n$data = '';\n$c = $entry[2];\n\nwhile ($c) {\nif ($c < 8192) {\n$data .= @fread($fp, $c);\n$c = 0;\n} else {\n$c -= 8192;\n$data .= @fread($fp, 8192);\n}\n}\n\nif ($entry[4] & self::GZ) {\n$data = gzinflate($data);\n} elseif ($entry[4] & self::BZ2) {\n$data = bzdecompress($data);\n}\n\nif (strlen($data) != $entry[0]) {\ndie(\"Invalid internal .phar file (size error \" . strlen($data) . \" != \" .\n$entry[0] . \")\");\n}\n\nif ($entry[3] != sprintf(\"%u\", crc32($data) & 0xffffffff)) {\ndie(\"Invalid internal .phar file (checksum error)\");\n}\n\nreturn $data;\n}\n\nstatic function _removeTmpFiles($temp, $origdir)\n{\nchdir($temp);\n\nforeach (glob('*') as $f) {\nif (file_exists($f)) {\nis_dir($f) ? @rmdir($f) : @unlink($f);\nif (file_exists($f) && is_dir($f)) {\nself::_removeTmpFiles($f, getcwd());\n}\n}\n}\n\n@rmdir($temp);\nclearstatcache();\nchdir($origdir);\n}\n}\n\nExtract_Phar::go();\n__HALT_COMPILER(); ?>";

-	static const int newstub_len = 6623;
+	static const int newstub_len = 6625;


Suggested change

static const int newstub_len = 6625;

static const size_t newstub_len = 6625;

For master maybe?

Girgias · 2024-01-15T09:40:05Z

ext/phar/makestub.php

 */

-static inline void phar_get_stub(const char *index_php, const char *web, size_t *len, char **stub, const int name_len, const int web_len)
+static inline zend_string *phar_get_stub(const char *index_php, const char *web, const int name_len, const int web_len)


Suggested change

static inline zend_string *phar_get_stub(const char *index_php, const char *web, const int name_len, const int web_len)

static inline zend_string *phar_get_stub(const char *index_php, const char *web, size_t name_len, size_t web_len)

It could make sense to also rearrange the parameter order in master alongside this.

Agreed that this should change to size_t as that's what is used in phar_create_default_stub.

Just a note that this will also require format change in strpprintf.

krakjoe · 2025-09-02T04:57:00Z

I switched the base branch to master and fixed conflicts so we can run CI ... the alternative simpler approach that was taken in #19643 was failing CI, so I figure we might as well revisit this more complete approach.

bukka

From reviewing the logic of this interesting templating solution, it looks correct to me.

Just one minor suggestion for the comment and that size_t part probably makes sense too.

bukka · 2025-09-02T10:40:08Z

ext/phar/makestub.php

+static inline zend_string *phar_get_stub(const char *index_php, const char *web, const int name_len, const int web_len)
 {
+	/* Do NOT modify this file directly!
+	 * Modify shortarc.php instead and use makestub.php to generate this file. */


It should probably note that shortarc.php is just for the PHP content and makestub.php for the C code. Maybe something like:

Instead modify shortarc.php to change PHP code or makestub.php to change C code and then use makestub.php to generate this file.

bukka · 2025-09-02T10:43:18Z

ext/phar/makestub.php

 */

-static inline void phar_get_stub(const char *index_php, const char *web, size_t *len, char **stub, const int name_len, const int web_len)
+static inline zend_string *phar_get_stub(const char *index_php, const char *web, const int name_len, const int web_len)


Agreed that this should change to size_t as that's what is used in phar_create_default_stub.

bukka · 2025-09-02T11:00:46Z

ext/phar/makestub.php

 */

-static inline void phar_get_stub(const char *index_php, const char *web, size_t *len, char **stub, const int name_len, const int web_len)
+static inline zend_string *phar_get_stub(const char *index_php, const char *web, const int name_len, const int web_len)


Just a note that this will also require format change in strpprintf.

nielsdos · 2025-10-01T14:27:09Z

I double checked this with a word-diff and it seems fine. Waiting for CI now before merging.

* Fixes to phar stub There were a couple of issues with the phar stub. It seems that in the past people did not realise they had to change shortarc.php and run makestub.php to generate the stubfile and instead they modified stub.h manually. This meant that there were a couple of mistakes in the stub which are fixed in this patch. In particular: * The title tag was not closed * The length of the stub was wrong in stub.h * This PR syncs previous changes to stub.h back to shortarc.php and vice versa. * Adds a note such that hopefully no mistakes against updating the stubs are made in the future (hopefully). * The makestub.php script was actually broken because the expected signature of the stub got changed in b874f1a * Verify that the phar stub is up-to-date in CI * Update stub again after master changes * size_t changes * Update test after stub changes (canonical casts) --------- Co-authored-by: Joe Watkins <[email protected]>

nielsdos force-pushed the phar-stub-fix branch from fc4e36d to 5ec502a Compare February 26, 2023 00:11

devnexen added the Extension: phar label Feb 26, 2023

Verify that the phar stub is up-to-date in CI

4a46354

Girgias reviewed Feb 26, 2023

View reviewed changes

nielsdos mentioned this pull request Apr 15, 2023

Fix ending tag of 'title' element #11079

Closed

Girgias approved these changes Jan 15, 2024

View reviewed changes

nielsdos mentioned this pull request Jul 21, 2025

Fix missing </title> in phar stub #19199

Closed

nielsdos mentioned this pull request Aug 30, 2025

Fix closing title tag #19643

Closed

krakjoe changed the base branch from PHP-8.1 to master September 2, 2025 04:53

krakjoe requested a review from TimWolla as a code owner September 2, 2025 04:53

Merge branch 'master' into phar-stub-fix

6cc480d

bukka reviewed Sep 2, 2025

View reviewed changes

nielsdos added 3 commits October 1, 2025 16:10

Merge branch 'master' into phar-stub-fix

f9f989e

Update stub again after master changes

50064fe

size_t changes

60f788c

Update test after stub changes (canonical casts)

326e2ee

nielsdos merged commit b32a2a5 into php:master Oct 1, 2025
8 of 9 checks passed

	static const int newstub_len = 6625;
	static const size_t newstub_len = 6625;

	static inline zend_string phar_get_stub(const char index_php, const char *web, const int name_len, const int web_len)
	static inline zend_string phar_get_stub(const char index_php, const char *web, size_t name_len, size_t web_len)

Fixes to phar stub #10706

Fixes to phar stub #10706

Uh oh!

Conversation

nielsdos commented Feb 25, 2023

Uh oh!

mvorisek commented Feb 26, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Feb 26, 2023

Uh oh!

medabkari commented Jan 14, 2024

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

Girgias Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

bukka Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

bukka Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

krakjoe commented Sep 2, 2025

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

bukka Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

bukka Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

bukka Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Oct 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants