Skip to content
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

Fix bug for 77204 #4127

Closed
wants to merge 1 commit into from
Closed

Fix bug for 77204 #4127

wants to merge 1 commit into from

Conversation

@peter279k
Copy link
Contributor

peter279k commented May 7, 2019

Changed log

  • Resolves #77204 in bugs.php.net.
@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented May 7, 2019

This changes an PHPAPI function, so it breaks extensions that depend on it. What we should do in this case is to rename php_getimagetype to php_getimagetype_ex and add a "new" function called php_getimagetype which is a PHPAPI and allow php_getimagetype_ex to be called with a NULL argument as this function can be used to read from streams where a filename may not always be available.

This way we keep compatibility. We do similar things with other exposed functionality with PHPAPI, you can find some examples in ext/standard.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented May 7, 2019

@KalleZ Unless there is evidence that these functions see heavy use outside of php-src, changing the signature is fine for 7.4.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented May 7, 2019

Though I do wonder -- is there any way to get the file (or stream) name from the stream itself?

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented May 7, 2019

@nikic I don't remember if there was a reliably way, but some streams may not expose a filename like a custom stream. For ext/exif I had to implement something like:
https://github.com/php/php-src/blob/master/ext/exif/exif.c#L4255

Now I do realize the userland function getimagesizes() only supports a filename argument.

@peter279k

This comment has been minimized.

Copy link
Contributor Author

peter279k commented May 7, 2019

This changes an PHPAPI function, so it breaks extensions that depend on it. What we should do in this case is to rename php_getimagetype to php_getimagetype_ex and add a "new" function called php_getimagetype which is a PHPAPI and allow php_getimagetype_ex to be called with a NULL argument as this function can be used to read from streams where a filename may not always be available.

This way we keep compatibility. We do similar things with other exposed functionality with PHPAPI, you can find some examples in ext/standard.

@KalleZ, thank you for your recommendation.

It's fine to use the another function to do this without changing the php_getimagetype function because of dependency.

Should I accept this request change?

@peter279k

This comment has been minimized.

Copy link
Contributor Author

peter279k commented May 7, 2019

@KalleZ Unless there is evidence that these functions see heavy use outside of php-src, changing the signature is fine for 7.4.

I just found that this php_getimagetype PHPAPI is only used in exif extension at this moment.

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented May 7, 2019

@peter279k Can you find any other sources of usage of this function in PECL, Github or around the web just to give an idea if this is even remotely used?

Thanks for your effort btw :)

@peter279k

This comment has been minimized.

Copy link
Contributor Author

peter279k commented May 8, 2019

@peter279k Can you find any other sources of usage of this function in PECL, Github or around the web just to give an idea if this is even remotely used?

Thanks for your effort btw :)

H @KalleZ, thank you for your reply.

After using the php_getimagetype and image to be keywords to find related funtions usage around PECL, GitHub and web, I find them.

But they're too old or seems to be unmaintained.

The lists I found are as follows:

Some libraries are found in GitHub look old and inactive.

I just present the captured shots to let us know that this function is available in exif and standard image extension :).

圖片

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented May 8, 2019

Thank you for your research. In that case I guess its fine to change the signature for PHP-7.4, however I would still like if the filename parameter could accept NULL for the cases explained above.

PHP's Stream API does not provide a reliable way to supply a file name unless its a standard I/O operation, in that case the caller may not have a file name to supply. Something as simple as checking if filename is NULL and if so, then setting it to a value like <stream>.

What do you think? cc @nikic

@peter279k

This comment has been minimized.

Copy link
Contributor Author

peter279k commented May 8, 2019

Thank you for your research. In that case I guess its fine to change the signature for PHP-7.4, however I would still like if the filename parameter could accept NULL for the cases explained above.

PHP's Stream API does not provide a reliable way to supply a file name unless its a standard I/O operation, in that case the caller may not have a file name to supply. Something as simple as checking if filename is NULL and if so, then setting it to a value like <stream>.

What do you think? cc @nikic

Hi @KalleZ, thank you for your reply.

Perhaps we can let the filename be optional argument in php_getimagetype function if possible.

ext/exif/exif.c Show resolved Hide resolved
@@ -1385,7 +1385,7 @@ PHPAPI int php_getimagetype(php_stream * stream, char *filetype)
}
/* }}} */

static void php_getimagesize_from_stream(php_stream *stream, zval *info, INTERNAL_FUNCTION_PARAMETERS) /* {{{ */
static void php_getimagesize_from_stream(php_stream *stream, char *input, zval *info, INTERNAL_FUNCTION_PARAMETERS) /* {{{ */

This comment has been minimized.

Copy link
@peter279k

peter279k May 8, 2019

Author Contributor
Suggested change
static void php_getimagesize_from_stream(php_stream *stream, char *input, zval *info, INTERNAL_FUNCTION_PARAMETERS) /* {{{ */
static void php_getimagesize_from_stream(php_stream *stream, zval *info, INTERNAL_FUNCTION_PARAMETERS) /* {{{ */
{
char tmp[12];
int twelve_bytes_read;

if ( !filetype) filetype = tmp;
if((php_stream_read(stream, filetype, 3)) != 3) {
php_error_docref(NULL, E_NOTICE, "Read error!");
php_error_docref(NULL, E_NOTICE, "Read error from %s!", input);

This comment has been minimized.

Copy link
@peter279k

peter279k May 8, 2019

Author Contributor
Suggested change
php_error_docref(NULL, E_NOTICE, "Read error from %s!", input);
if (!input) {
php_error_docref(NULL, E_NOTICE, "Read error!");
} else {
php_error_docref(NULL, E_NOTICE, "Read error from %s!", input);
}
@peter279k

This comment has been minimized.

Copy link
Contributor Author

peter279k commented May 8, 2019

@KalleZ, I also add some suggested code snippets.

Are the request changes that you expect :)?

@petk petk added the Bugfix label May 16, 2019
@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented May 24, 2019

@kalle can you wrap this one up please ?

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 2, 2019

To finalize this, here would be my suggestions:

  • Keep input a required parameter of php_getimagetype(). Making it optional makes printing code harder.
  • Change the error message to Error reading from %s.
  • For the getimagesizefromstring case make the filename "string", so the error message becomes Error reading from string.
@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Oct 2, 2019

@peter279k can we get a status update here please ?

@peter279k

This comment has been minimized.

Copy link
Contributor Author

peter279k commented Oct 2, 2019

@peter279k can we get a status update here please ?

@krakjoe, thanks for your reply. I will do latest final changes that @nikic mentions.

@peter279k peter279k force-pushed the peter279k:bug_77204 branch from b6785ae to 707d483 Oct 2, 2019
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Oct 2, 2019

Thanks! Merged as 1ea329d into master, as we can't make ABI changes in 7.4 anymore.

@nikic nikic closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.