Skip to content

Commit

Permalink
ext/phar: Voidify flush function as it always returns EOL
Browse files Browse the repository at this point in the history
  • Loading branch information
@Girgias
Girgias committed Sep 14, 2024
1 parent 4f9fdf8 commit 2513258
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 66 deletions.
41 changes: 20 additions & 21 deletions ext/phar/phar.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2520,16 +2520,16 @@ zend_string *phar_create_default_stub(const char *index_php, const char *web_ind
}
/* }}} */

int phar_flush(phar_archive_data *phar, char **error) {
return phar_flush_ex(phar, NULL, false, error);
void phar_flush(phar_archive_data *phar, char **error) {
phar_flush_ex(phar, NULL, false, error);
}

/**
* Save phar contents to disk
*
* if user_stub is NULL the default or existing stub should be used
*/
int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_default_stub, char **error) /* {{{ */
void phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_default_stub, char **error) /* {{{ */
{
static const char halt_stub[] = "__HALT_COMPILER();";

Expand Down Expand Up @@ -2557,29 +2557,31 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 0, "internal error: attempt to flush cached zip-based phar \"%s\"", phar->fname);
}
return EOF;
return;
}

if (error) {
*error = NULL;
}

if (!zend_hash_num_elements(&phar->manifest) && !user_stub) {
return EOF;
return;
}

zend_hash_clean(&phar->virtual_dirs);

if (phar->is_zip) {
return phar_zip_flush(phar, user_stub, is_default_stub, error);
phar_zip_flush(phar, user_stub, is_default_stub, error);
return;
}

if (phar->is_tar) {
return phar_tar_flush(phar, user_stub, is_default_stub, error);
phar_tar_flush(phar, user_stub, is_default_stub, error);
return;
}

if (PHAR_G(readonly)) {
return EOF;
return;
}

if (phar->fp && !phar->is_brandnew) {
Expand All @@ -2598,7 +2600,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (must_close_old_file) {
php_stream_close(oldfile);
}
return EOF;
return;
}

if (user_stub) {
Expand All @@ -2612,7 +2614,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 0, "illegal stub for phar \"%s\" (__HALT_COMPILER(); is missing)", phar->fname);
}
return EOF;
return;
}

size_t len = pos - ZSTR_VAL(user_stub) + strlen(halt_stub);
Expand All @@ -2630,7 +2632,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 0, "unable to create stub from string in new phar \"%s\"", phar->fname);
}
return EOF;
return;
}
phar->halt_offset = len + end_sequence_len;
} else {
Expand Down Expand Up @@ -2660,7 +2662,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (new_stub) {
zend_string_free(new_stub);
}
return EOF;
return;
}
if (new_stub) {
zend_string_free(new_stub);
Expand Down Expand Up @@ -2756,7 +2758,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 0, "unable to seek to start of file \"%s\" while creating new phar \"%s\"", entry->filename, phar->fname);
}
return EOF;
return;
}
newcrc32 = php_crc32_bulk_init();
php_crc32_stream_bulk_update(&newcrc32, file, entry->uncompressed_filesize);
Expand All @@ -2782,7 +2784,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
spprintf(error, 0, "unable to bzip2 compress file \"%s\" to new phar \"%s\"", entry->filename, phar->fname);
}
}
return EOF;
return;
}

/* create new file that holds the compressed versions */
Expand Down Expand Up @@ -3105,7 +3107,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
php_stream_close(oldfile);
}
php_stream_close(newfile);
return EOF;
return;
}

php_stream_write(newfile, digest, digest_len);
Expand Down Expand Up @@ -3157,7 +3159,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 4096, "unable to open new phar \"%s\" for writing", phar->fname);
}
return EOF;
return;
}

if (phar->flags & PHAR_FILE_COMPRESSED_GZ) {
Expand All @@ -3173,7 +3175,7 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 4096, "unable to compress all contents of phar \"%s\" using zlib, PHP versions older than 5.2.6 have a buggy zlib", phar->fname);
}
return EOF;
return;
}

php_stream_filter_append(&phar->fp->writefilters, filter);
Expand Down Expand Up @@ -3203,10 +3205,9 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
if (error) {
spprintf(error, 0, "unable to seek to __HALT_COMPILER(); in new phar \"%s\"", phar->fname);
}
return EOF;
}

return EOF;
return;

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 Sep 14, 2024

Member

This should actually be return 0; to signal success (same for phar_zip_flush() and phar_tar_flush() below), and callers should check for success/failure. (Could alternatively return ZEND_RESULT_CODE, but 0/EOF is more in line with fflush().)

"If a function be advertised to return an error code in the event of difficulties, thou shalt check for that code, yea, even though the checks triple the size of thy code and produce aches in thy typing fingers, for if thou thinkest it cannot happen to me, the gods shall surely punish thee for thy arrogance." – Henry Spencer

This comment has been minimized.

Sign in to view

Copy link
@Girgias

Girgias Sep 14, 2024

Author Member

I would have guessed that it should return something else on success, but considering it has had this behaviour forever I didn't want to change it.

I think I will continue to do incremental clean-up refactoring in ext/phar so that figuring out the correct behaviour is easier down the line.

cleanup:
if (shared_cfp != NULL) {
Expand All @@ -3218,8 +3219,6 @@ int phar_flush_ex(phar_archive_data *phar, zend_string *user_stub, bool is_defau
entry->header_offset = 0;
}
} ZEND_HASH_FOREACH_END();

return EOF;
}
/* }}} */

Expand Down
8 changes: 4 additions & 4 deletions ext/phar/phar_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,12 +447,12 @@ zend_result phar_copy_on_write(phar_archive_data **pphar);
bool phar_is_tar(char *buf, char *fname);
zend_result phar_parse_tarfile(php_stream* fp, char *fname, size_t fname_len, char *alias, size_t alias_len, phar_archive_data** pphar, uint32_t compression, char **error);
zend_result phar_open_or_create_tar(char *fname, size_t fname_len, char *alias, size_t alias_len, int is_data, uint32_t options, phar_archive_data** pphar, char **error);
int phar_tar_flush(phar_archive_data *phar, zend_string *user_stub, bool is_default_stub, char **error);
void phar_tar_flush(phar_archive_data *phar, zend_string *user_stub, bool is_default_stub, char **error);

/* zip functions in zip.c */
int phar_parse_zipfile(php_stream *fp, char *fname, size_t fname_len, char *alias, size_t alias_len, phar_archive_data** pphar, char **error);
int phar_open_or_create_zip(char *fname, size_t fname_len, char *alias, size_t alias_len, int is_data, uint32_t options, phar_archive_data** pphar, char **error);
int phar_zip_flush(phar_archive_data *archive, zend_string *user_stub, bool is_default_stub, char **error);
void phar_zip_flush(phar_archive_data *archive, zend_string *user_stub, bool is_default_stub, char **error);

#ifdef PHAR_MAIN
extern const php_stream_wrapper php_stream_phar_wrapper;
Expand All @@ -468,8 +468,8 @@ phar_entry_info *phar_get_entry_info(phar_archive_data *phar, char *path, size_t
phar_entry_info *phar_get_entry_info_dir(phar_archive_data *phar, char *path, size_t path_len, char dir, char **error, int security);
phar_entry_data *phar_get_or_create_entry_data(char *fname, size_t fname_len, char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security);
zend_result phar_get_entry_data(phar_entry_data **ret, char *fname, size_t fname_len, char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security);
int phar_flush_ex(phar_archive_data *archive, zend_string *user_stub, bool is_default_stub, char **error);
int phar_flush(phar_archive_data *archive, char **error);
void phar_flush_ex(phar_archive_data *archive, zend_string *user_stub, bool is_default_stub, char **error);
void phar_flush(phar_archive_data *archive, char **error);
zend_result phar_detect_phar_fname_ext(const char *filename, size_t filename_len, const char **ext_str, size_t *ext_len, int executable, int for_create, int is_complete);
zend_result phar_split_fname(const char *filename, size_t filename_len, char **arch, size_t *arch_len, char **entry, size_t *entry_len, int executable, int for_create);

Expand Down
5 changes: 2 additions & 3 deletions ext/phar/stream.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -469,17 +469,16 @@ static ssize_t phar_stream_write(php_stream *stream, const char *buf, size_t cou
static int phar_stream_flush(php_stream *stream) /* {{{ */
{
char *error;
int ret;
phar_entry_data *data = (phar_entry_data *) stream->abstract;

if (data->internal_file->is_modified) {
data->internal_file->timestamp = time(0);
ret = phar_flush(data->phar, &error);
phar_flush(data->phar, &error);
if (error) {
php_stream_wrapper_log_error(stream->wrapper, REPORT_ERRORS, "%s", error);
efree(error);
}
return ret;
return EOF;
} else {
return EOF;
}
Expand Down
Loading

0 comments on commit 2513258

Please sign in to comment.