Skip to content

Add support for upload files from buffer string in curl extenion #1283

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Add support for upload files from buffer string in curl extenion #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6319f95
curl: add support to upload files from buffer string
Irker May 8, 2015
88e1ae0
Merge branch 'master' into curl_buffer_file
Irker May 16, 2015
17ea5ca
Merge branch 'master' into curl_buffer_file
Irker Nov 29, 2015
4e4fec6
Merge branch 'master' into curl_buffer_file
Irker Mar 12, 2016
e59e9de
remove excess arguments from curlfile_ctor
Irker Mar 12, 2016
afbe03b
fix curl_buffer_file_create doc comment
Irker Mar 12, 2016
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
104 changes: 84 additions & 20 deletions ext/curl/curl_file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,44 +29,75 @@

PHP_CURL_API zend_class_entry *curl_CURLFile_class;

static void curlfile_ctor(INTERNAL_FUNCTION_PARAMETERS)
static void curlfile_ctor(char *fname, size_t fname_len, char *mime, size_t mime_len, char *postname, size_t postname_len, char *buffer, size_t buffer_len, zval *return_value)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you need INTERNAL_FUNCTION_PARAMETERS? You don't seem to use them except for return_value. So maybe just pass return_value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, will change it.

char *fname = NULL, *mime = NULL, *postname = NULL;
size_t fname_len, mime_len, postname_len;
zval *cf = return_value;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "s|ss", &fname, &fname_len, &mime, &mime_len, &postname, &postname_len) == FAILURE) {
return;
}

if (fname) {
zend_update_property_string(curl_CURLFile_class, cf, "name", sizeof("name")-1, fname);
zend_update_property_stringl(curl_CURLFile_class, cf, "name", sizeof("name")-1, fname, fname_len);
}

if (mime) {
zend_update_property_string(curl_CURLFile_class, cf, "mime", sizeof("mime")-1, mime);
zend_update_property_stringl(curl_CURLFile_class, cf, "mime", sizeof("mime")-1, mime, mime_len);
}

if (postname) {
zend_update_property_string(curl_CURLFile_class, cf, "postname", sizeof("postname")-1, postname);
zend_update_property_stringl(curl_CURLFile_class, cf, "postname", sizeof("postname")-1, postname, postname_len);
}

if (buffer) {
zend_update_property_stringl(curl_CURLFile_class, cf, "buffer", sizeof("buffer")-1, buffer, buffer_len);
}
}

/* {{{ proto void CURLFile::__construct(string $name, [string $mimetype [, string $postfilename]])
/* {{{ proto void CURLFile::__construct([string $name, [string $mimetype [, string $postfilename]]])
Create the CURLFile object */
ZEND_METHOD(CURLFile, __construct)
{
char *fname = NULL, *mime = NULL, *postname = NULL, *buffer = NULL;
size_t fname_len, mime_len, postname_len, buffer_len;

return_value = getThis();
curlfile_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|sss", &fname, &fname_len, &mime, &mime_len, &postname, &postname_len) == FAILURE) {
return;
}

curlfile_ctor(fname, fname_len, mime, mime_len, postname, postname_len, buffer, buffer_len, return_value);
}
/* }}} */

/* {{{ proto CURLFile curl_file_create(string $name, [string $mimetype [, string $postfilename]])
Create the CURLFile object */
Create the CURLFile object from file */
PHP_FUNCTION(curl_file_create)
{
object_init_ex( return_value, curl_CURLFile_class );
curlfile_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
char *fname = NULL, *mime = NULL, *postname = NULL, *buffer = NULL;
size_t fname_len, mime_len, postname_len, buffer_len;

object_init_ex( return_value, curl_CURLFile_class );

if (zend_parse_parameters(ZEND_NUM_ARGS(), "s|ss", &fname, &fname_len, &mime, &mime_len, &postname, &postname_len) == FAILURE) {
return;
}

curlfile_ctor(fname, fname_len, mime, mime_len, postname, postname_len, buffer, buffer_len, return_value);
}
/* }}} */

/* {{{ proto CURLFile curl_buffer_file_create(string $buffer, string $postfilename, [string $mimetype])
Create the CURLFile object from string buffer */
Copy link
Contributor

Choose a reason for hiding this comment

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

it is kind of confusing that curl_file_create takes name, mimetype, filename but curl_buffer_file_create takes buffer, filename, mimetype - two last arguments are kind of switched. I understand why it may be but I foresee people complaining about it. Not sure what to do here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. We have only two variants:

  1. keep current arguments order, because for buffer we always require postname
  2. keep old arguments order with "null" value $mimetype argument skip, like in mb_substr etc.: curl_buffer_file_create($buffer, null, $postfilename)

PHP_FUNCTION(curl_buffer_file_create)
{
char *fname = NULL, *mime = NULL, *postname = NULL, *buffer = NULL;
size_t fname_len, mime_len, postname_len, buffer_len;

object_init_ex( return_value, curl_CURLFile_class );

if (zend_parse_parameters(ZEND_NUM_ARGS(), "ss|s", &buffer, &buffer_len, &postname, &postname_len, &mime, &mime_len) == FAILURE) {
return;
}

curlfile_ctor(fname, fname_len, mime, mime_len, postname, postname_len, buffer, buffer_len, return_value);
}
/* }}} */

Expand All @@ -90,7 +121,7 @@ static void curlfile_set_property(char *name, INTERNAL_FUNCTION_PARAMETERS)
if (zend_parse_parameters(ZEND_NUM_ARGS(), "s", &arg, &arg_len) == FAILURE) {
return;
}
zend_update_property_string(curl_CURLFile_class, getThis(), name, strlen(name), arg);
zend_update_property_stringl(curl_CURLFile_class, getThis(), name, strlen(name), arg, arg_len);
}

/* {{{ proto string CURLFile::getFilename()
Expand All @@ -101,6 +132,14 @@ ZEND_METHOD(CURLFile, getFilename)
}
/* }}} */

/* {{{ proto string CURLFile::getBuffer()
Get buffer */
ZEND_METHOD(CURLFile, getBuffer)
{
curlfile_get_property("buffer", INTERNAL_FUNCTION_PARAM_PASSTHRU);
}
/* }}} */

/* {{{ proto string CURLFile::getMimeType()
Get MIME type */
ZEND_METHOD(CURLFile, getMimeType)
Expand All @@ -117,6 +156,22 @@ ZEND_METHOD(CURLFile, getPostFilename)
}
/* }}} */

/* {{{ proto void CURLFile::setFilename(string $name)
Set file name */
ZEND_METHOD(CURLFile, setFilename)
{
curlfile_set_property("name", INTERNAL_FUNCTION_PARAM_PASSTHRU);
}
/* }}} */

/* {{{ proto void CURLFile::setBuffer(string $buffer)
Set buffer */
ZEND_METHOD(CURLFile, setBuffer)
{
curlfile_set_property("buffer", INTERNAL_FUNCTION_PARAM_PASSTHRU);
}
/* }}} */
Copy link
Contributor

Choose a reason for hiding this comment

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

What if somebody sets both buffer and filename? Not clear what happens in this case - would the last one win? Would filename/buffer always win?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filename always win.. Are you mean make more clear documentation or make more clear API? For example throw error if we setup both params.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if filename always wins then maybe setting buffer while filename is already set should produce an exception. Though then it is not clear why these methods are needed at all - can't you just construct the proper one via factory method? Why one would need to change it on existing object? I'd much rather prefer it to be immutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I fully agree with you! Immutable objects will be perfect for CURLFile. But it greatly breaks backward compatibility.
Developers can do all this things:
curl_buffer_file_create (...)
new CURLFile(...)
$CURLFile->setFilename = '...'
$CURLFile->name=... ( it's public! )

So if we try to make CURLFile immutable object we required RFC, and many chance that it can't be approved by community.
For not popular small feature to send file from string we will broke BC.
I tried to change as less as possible to force pull request applied. But If you support me, I can try to create an RFC or a mail thread in internals to discuss this idea.


/* {{{ proto void CURLFile::setMimeType(string $mime)
Set MIME type */
ZEND_METHOD(CURLFile, setMimeType)
Expand All @@ -137,12 +192,17 @@ ZEND_METHOD(CURLFile, setPostFilename)
Unserialization handler */
ZEND_METHOD(CURLFile, __wakeup)
{
zend_update_property_string(curl_CURLFile_class, getThis(), "name", sizeof("name")-1, "");
zend_throw_exception(NULL, "Unserialization of CURLFile instances is not allowed", 0);
zval *fname, rv;

fname = zend_read_property(curl_CURLFile_class, getThis(), "name", strlen("name"), 1, &rv);
if (zval_is_true(fname)) {
zend_update_property_string(curl_CURLFile_class, getThis(), "name", sizeof("name")-1, "");
zend_throw_exception(NULL, "Unserialization of CURLFile instances with file name is not allowed", 0);
}
}
/* }}} */

ZEND_BEGIN_ARG_INFO_EX(arginfo_curlfile_create, 0, 0, 1)
ZEND_BEGIN_ARG_INFO_EX(arginfo_curlfile_ctor, 0, 0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why rename it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

curl_file_create in interface.c use same name 'arginfo_curlfile_create' for arg info. So because I added new arg info for function curl_buffer_file_create I decided to rename arg info for constructor.

ZEND_ARG_INFO(0, filename)
ZEND_ARG_INFO(0, mimetype)
ZEND_ARG_INFO(0, postname)
Expand All @@ -154,8 +214,11 @@ ZEND_END_ARG_INFO()


static const zend_function_entry curlfile_funcs[] = {
PHP_ME(CURLFile, __construct, arginfo_curlfile_create, ZEND_ACC_CTOR|ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, __construct, arginfo_curlfile_ctor, ZEND_ACC_CTOR|ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, getFilename, NULL, ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, setFilename, arginfo_curlfile_name, ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, getBuffer, NULL, ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, setBuffer, arginfo_curlfile_name, ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, getMimeType, NULL, ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, setMimeType, arginfo_curlfile_name, ZEND_ACC_PUBLIC)
PHP_ME(CURLFile, getPostFilename, NULL, ZEND_ACC_PUBLIC)
Expand All @@ -170,6 +233,7 @@ void curlfile_register_class(void)
INIT_CLASS_ENTRY( ce, "CURLFile", curlfile_funcs );
curl_CURLFile_class = zend_register_internal_class(&ce);
zend_declare_property_string(curl_CURLFile_class, "name", sizeof("name")-1, "", ZEND_ACC_PUBLIC);
zend_declare_property_string(curl_CURLFile_class, "buffer", sizeof("buffer")-1, "", ZEND_ACC_PUBLIC);
zend_declare_property_string(curl_CURLFile_class, "mime", sizeof("mime")-1, "", ZEND_ACC_PUBLIC);
zend_declare_property_string(curl_CURLFile_class, "postname", sizeof("postname")-1, "", ZEND_ACC_PUBLIC);
}
Expand Down
87 changes: 62 additions & 25 deletions ext/curl/interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,12 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_curlfile_create, 0, 0, 1)
ZEND_ARG_INFO(0, mimetype)
ZEND_ARG_INFO(0, postname)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_curlbufferfile_create, 0, 0, 2)
ZEND_ARG_INFO(0, buffer)
ZEND_ARG_INFO(0, postname)
ZEND_ARG_INFO(0, mimetype)
ZEND_END_ARG_INFO()
/* }}} */

/* {{{ curl_functions[]
Expand Down Expand Up @@ -471,6 +477,7 @@ const zend_function_entry curl_functions[] = {
PHP_FE(curl_share_close, arginfo_curl_share_close)
PHP_FE(curl_share_setopt, arginfo_curl_share_setopt)
PHP_FE(curl_file_create, arginfo_curlfile_create)
PHP_FE(curl_buffer_file_create, arginfo_curlbufferfile_create)
PHP_FE_END
};
/* }}} */
Expand Down Expand Up @@ -2543,34 +2550,64 @@ static int _php_curl_setopt(php_curl *ch, zend_long option, zval *zvalue) /* {{{
zval *prop, rv;
char *type = NULL, *filename = NULL;

prop = zend_read_property(curl_CURLFile_class, current, "name", sizeof("name")-1, 0, &rv);
if (Z_TYPE_P(prop) != IS_STRING) {
php_error_docref(NULL, E_WARNING, "Invalid filename for key %s", ZSTR_VAL(string_key));
} else {
postval = Z_STR_P(prop);
prop = zend_read_property(curl_CURLFile_class, current, "mime", sizeof("mime")-1, 0, &rv);
if (Z_TYPE_P(prop) == IS_STRING && Z_STRLEN_P(prop) > 0) {
type = Z_STRVAL_P(prop);
}

if (php_check_open_basedir(ZSTR_VAL(postval))) {
return 1;
}
prop = zend_read_property(curl_CURLFile_class, current, "postname", sizeof("postname")-1, 0, &rv);
if (Z_TYPE_P(prop) == IS_STRING && Z_STRLEN_P(prop) > 0) {
filename = Z_STRVAL_P(prop);
}

prop = zend_read_property(curl_CURLFile_class, current, "mime", sizeof("mime")-1, 0, &rv);
if (Z_TYPE_P(prop) == IS_STRING && Z_STRLEN_P(prop) > 0) {
type = Z_STRVAL_P(prop);
}
prop = zend_read_property(curl_CURLFile_class, current, "postname", sizeof("postname")-1, 0, &rv);
if (Z_TYPE_P(prop) == IS_STRING && Z_STRLEN_P(prop) > 0) {
filename = Z_STRVAL_P(prop);
prop = zend_read_property(curl_CURLFile_class, current, "name", sizeof("name")-1, 0, &rv);
if (zval_is_true(prop)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand why zval_is_true check? If you wanted to check for name being empty/null, then it should be that check. Otherwise you'll get in trouble with "0" etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smalyshev , can it be something like if (!Z_ISNULL_P(prop) && (Z_TYPE_P(prop) != IS_STRING || Z_STRLEN_P(prop) > 0)) { ? Not sure if prop can be ISUNDEF after zend_read_property. Can you correct me, before I will push this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it can be UNDEF, but I'd rather specify explicitly which values we do expect - i.e. if we expect name to be NULL when we use buffer.

Also, whatever check you do here you should also do on __wakeup. In fact, I'd make it a macro or inline function to ensure they are always the same.

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 we will use NULL as base value for this condition, we must change this code: zend_declare_property_string(curl_CURLFile_class, "name", sizeof("name")-1, "", ZEND_ACC_PUBLIC); to zend_declare_property_null. This is small but BC break =(, because we change default value for property.

Previous variant of pull-request with separate CURLBufferFile seems now less terrible )))

/* upload from "name" */
if (Z_TYPE_P(prop) != IS_STRING) {
php_error_docref(NULL, E_WARNING, "Invalid filename for key %s", ZSTR_VAL(string_key));
} else {
postval = Z_STR_P(prop);

if (php_check_open_basedir(ZSTR_VAL(postval))) {
return 1;
}

form_error = curl_formadd(&first, &last,
CURLFORM_COPYNAME, ZSTR_VAL(string_key),
CURLFORM_NAMELENGTH, ZSTR_LEN(string_key),
CURLFORM_FILENAME, filename ? filename : ZSTR_VAL(postval),
CURLFORM_CONTENTTYPE, type ? type : "application/octet-stream",
CURLFORM_FILE, ZSTR_VAL(postval),
CURLFORM_END);

if (form_error != CURL_FORMADD_OK) {
/* Not nice to convert between enums but we only have place for one error type */
error = (CURLcode)form_error;
}
}
form_error = curl_formadd(&first, &last,
CURLFORM_COPYNAME, ZSTR_VAL(string_key),
CURLFORM_NAMELENGTH, ZSTR_LEN(string_key),
CURLFORM_FILENAME, filename ? filename : ZSTR_VAL(postval),
CURLFORM_CONTENTTYPE, type ? type : "application/octet-stream",
CURLFORM_FILE, ZSTR_VAL(postval),
CURLFORM_END);
if (form_error != CURL_FORMADD_OK) {
/* Not nice to convert between enums but we only have place for one error type */
error = (CURLcode)form_error;
} else {
/* upload from "buffer" */
prop = zend_read_property(curl_CURLFile_class, current, "buffer", sizeof("buffer")-1, 0, &rv);
if (Z_TYPE_P(prop) != IS_STRING) {
php_error_docref(NULL, E_WARNING, "Invalid buffer for key %s", string_key->val);
} else if(!filename) {
php_error_docref(NULL, E_WARNING, "Invalid post file name for key %s", string_key->val);
} else {
postval = Z_STR_P(prop);

form_error = curl_formadd(&first, &last,
CURLFORM_COPYNAME, ZSTR_VAL(string_key),
CURLFORM_NAMELENGTH, ZSTR_LEN(string_key),
CURLFORM_BUFFER, filename,
CURLFORM_BUFFERPTR, ZSTR_VAL(postval),
CURLFORM_BUFFERLENGTH, ZSTR_LEN(postval),
CURLFORM_CONTENTTYPE, type ? type : "application/octet-stream",
CURLFORM_END);

if (form_error != CURL_FORMADD_OK) {
/* Not nice to convert between enums but we only have place for one error type */
error = (CURLcode)form_error;
}
}
}

Expand Down
1 change: 1 addition & 0 deletions ext/curl/php_curl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ PHP_FUNCTION(curl_multi_setopt);
PHP_FUNCTION(curl_pause);
#endif
PHP_FUNCTION(curl_file_create);
PHP_FUNCTION(curl_buffer_file_create);


void _php_curl_multi_close(zend_resource *);
Expand Down
23 changes: 21 additions & 2 deletions ext/curl/tests/curl_file_serialize.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,30 @@ if (!extension_loaded("curl")) {
?>
--FILE--
<?php
$data = 'a:2:{s:4:"file";O:8:"CURLFile":3:{s:4:"name";s:13:"testdata1.txt";s:4:"mime";s:0:"";s:8:"postname";s:0:"";}s:4:"data";s:3:"foo";}';
$data = 'a:2:{s:4:"file";O:8:"CURLFile":4:{s:4:"name";s:0:"";s:6:"buffer";s:9:"testdata2";s:4:"mime";s:9:"mime_type";s:8:"postname";s:9:"post_name";}s:4:"data";s:3:"foo";}';
var_dump(unserialize($data));

$data = 'a:2:{s:4:"file";O:8:"CURLFile":4:{s:4:"name";s:6:"buffer";s:0:"";s:13:"testdata1.txt";s:4:"mime";s:0:"";s:8:"postname";s:0:"";}s:4:"data";s:3:"foo";}';
var_dump(unserialize($data));
?>
--EXPECTF--
Fatal error: Uncaught Exception: Unserialization of CURLFile instances is not allowed in %s
array(2) {
["file"]=>
object(CURLFile)#1 (4) {
["name"]=>
string(0) ""
["buffer"]=>
string(9) "testdata2"
["mime"]=>
string(9) "mime_type"
["postname"]=>
string(9) "post_name"
}
["data"]=>
string(3) "foo"
}

Fatal error: Uncaught Exception: Unserialization of CURLFile instances with file name is not allowed in %s
Stack trace:
#0 [internal function]: CURLFile->__wakeup()
#1 %s
Expand Down
Loading