-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix bug #73457 and add a test case #3466
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
Conversation
ext/standard/ftp_fopen_wrapper.c
Outdated
@@ -430,6 +430,7 @@ php_stream * php_stream_url_wrap_ftp(php_stream_wrapper *wrapper, const char *pa | |||
int8_t read_write = 0; | |||
char *transport; | |||
int transport_len; | |||
zend_string *error_message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like php_stream_xport_create()
does not NULL this variable out, so it will need a NULL initialization here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if php_stream_xport_create() did the null initialization, we'd still need the =NULL here, because an error might occur prior to the php_stream_xport_create() call. An alternative would be to move the error_string handling directly into the failure branch of the php_stream_xport_create() call, rather than having it in errexit. Either way should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, NULL initialization is required. That has been fixed now.
ext/standard/ftp_fopen_wrapper.c
Outdated
if (error_message) { | ||
php_stream_wrapper_log_error(wrapper, options, "Failed to set up data channel: %s", ZSTR_VAL(error_message)); | ||
zend_string_release(error_message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Indentation
Merged as 742783c into 7.1+. Thanks! |
Bug #73457