cURL: make possible to send file from buffer string #6456

Irker · 2020-11-26T07:51:09Z

In my practice, very often I needed to send a file that was created in the current script and stored in memory. Most often these were small generated images, PDF documents, XML files, and etc.

At the current moment in PHP we can send a file from a string variable only through a temporary file on a file system(CURLFile) or with unnecessary base64 converting:

$file_stream = 'data://application/octet-stream;base64,' . base64_encode($file);
$curlFile = new \CURLFile($path);
$curlFile->setPostFilename($file_name);

However, libcurl itself has an easy way to send a file from a string.

Since sending a file in this way always requires passing the file name, it will not work to embed this functionality into an existing CurlFile class without losing backward compatibility. Therefore, I decided to create a new class CURLStringFile.

Usage:

$curlFile = new CURLStringFile($file_content, $postname, $mime);

ext/curl/curl.stub.php

ext/curl/curl_string_file.stub.php

nikic

Sorry for the delay! This looks like a nice addition to me. I have one safety concern regarding the CURLFORM_BUFFERPTR use. And a more general design concern on whether we need all those methods on CURLStringFile at all, and can't just use typed properties.

Maybe @cmb69 or @kocsismate have thoughts on this addition as well?

ext/curl/curl.stub.php

ext/curl/interface.c

nikic · 2021-01-25T15:42:06Z

ext/curl/interface.c

+                            CURLFORM_BUFFER, filename,
+                            CURLFORM_CONTENTTYPE, type ? type : "application/octet-stream",
+                            CURLFORM_BUFFERPTR, ZSTR_VAL(postval),
+                            CURLFORM_BUFFERLENGTH, ZSTR_LEN(postval),


I don't think this is safe. According to curl docs:

is used in combination with CURLFORM_BUFFER. The parameter is a pointer to the buffer to be uploaded. This buffer must not be freed until after curl_easy_cleanup is called. You must also use CURLFORM_BUFFERLENGTH to set the number of bytes in the buffer.

This means that curl is not making a copy of the buffer. If the CURLStringFile object is destroyed before the request is sent, this will probably result in use-after-free.

Hi @nikic! Great thanks for the code review!
This issue scares me because looks like I need to copy the data somewhere inside CurlHandle. And also copy them when cloning, and destroy them in the curl destructor. Most likely I will not have enough knowledge in the C language.
I can suggest adding CurlStringFile only for curl >= 7.56.0. Or someone will help me with this issue.
Right now, I have code changes based on all your comments, except this one.

Nevermind. I tried to do it myself. =)

ext/curl/interface.c

nikic · 2021-01-25T15:46:22Z

ext/curl/tests/curl_string_file_upload.phpt

+--FILE--
+<?php
+
+function testcurl($ch, $postname, $data, $mime = '')


This should $mime = null and pass it unconditionally rather than checking empty

ext/opcache/Optimizer/zend_func_info.c

nikic · 2021-01-25T15:49:47Z

ext/curl/curl_string_file.stub.php

+    public function getData() {}
+
+    /** @return string */
+    public function getPostFilename() {}


CURLFile also has a setPostFilename method.

Though, taking a step back here, seeing as the class has public properties, these getters and setters are really redundant. I think we'd be better off declaring them as typed properties and just omitting the methods entirely.

It depends on whether we need to have the same interface as CURLFile. It doesn't seem necessary to me, as I would expect the common usage to just construct CURLStringFile and directly pass it to curl.

cmb69 · 2021-01-25T16:04:23Z

ext/curl/tests/curl_string_file_upload.phpt

+string(%d) "foo.txt"
+string(%d) "62942c05ed0d1b501c4afe6dc1c4db1b"
+string(%d) "foo.txt|text/plain|62942c05ed0d1b501c4afe6dc1c4db1b"
+string(%d) "foo.txt|text/plain|62942c05ed0d1b501c4afe6dc1c4db1b"


Nit: missing newline

Irker · 2021-01-30T12:01:36Z

@nikic @cmb69
Thanks again for the code review.
I updated the patch in which I tried to accommodate all your suggestions.
However, I'm not sure about some of my solutions:

I made a mime parameter with null as default like in CurlFile. Null is then replaced with "application/octet-stream". But we can make this value default right in the class instead of null.
I'm really not sure if I did everything right with ch-> to_free-> buffers
I accidentally discovered a bug that CurlFile has. If an object property zval is converted to reference ($postname =& $file->postname), then this file property will not pass validation.
I added checks like this:

if (Z_TYPE_P (prop) == IS_REFERENCE) {
    prop = Z_REFVAL_P (prop);
}

Not sure if this is correct.

As pointed out on GH-6456.

nikic · 2021-02-02T09:37:57Z

ext/curl/curl_string_file.c

+		Z_PARAM_STR_OR_NULL(mime)
+	ZEND_PARSE_PARAMETERS_END();
+
+	zend_update_property_stringl(curl_CURLStringFile_class, Z_OBJ_P(object), "data", sizeof("data")-1, ZSTR_VAL(data), ZSTR_LEN(data));


This will perform a copy of the data. You can use zend_update_property_str to avoid the copy.

nikic · 2021-02-02T09:38:29Z

ext/curl/curl_string_file.c

+	ZEND_PARSE_PARAMETERS_END();
+
+	zend_update_property_stringl(curl_CURLStringFile_class, Z_OBJ_P(object), "data", sizeof("data")-1, ZSTR_VAL(data), ZSTR_LEN(data));
+	zend_update_property_string(curl_CURLStringFile_class, Z_OBJ_P(object), "postname", sizeof("postname")-1, ZSTR_VAL(postname));


Same here -- unless this is intentionally written this way to truncate the filename at a null byte?

I did it for mime and postname the same way as for CURLFile. I think it required to be truncated.
But what the best way? Truncate or, for example, throw an exception if we found null-byte, or do something else?

I think it's okay to stick with the CURLFile behavior here.

nikic · 2021-02-02T09:39:29Z

ext/curl/curl_string_file.stub.php

+class CURLStringFile
+{
+    public string $data = "";
+    public string $postname = "";


You can leave off the default values on these properties (and on $mime as well if you assign it unconditionally).

nikic · 2021-02-02T09:39:58Z

ext/curl/interface.c

+
+			if (Z_TYPE_P(prop) == IS_REFERENCE) {
+				prop = Z_REFVAL_P(prop);
+			}


ZVAL_DEREF(prop) is simpler :)

nikic · 2021-02-02T09:45:10Z

@nikic @cmb69
Thanks again for the code review.
I updated the patch in which I tried to accommodate all your suggestions.
However, I'm not sure about some of my solutions:
1. I made a mime parameter with null as default like in CurlFile. Null is then replaced with "application/octet-stream". But we can make this value default right in the class instead of null.

Directly using "application/octect-stream" as the default makes sense to me.

2. I'm really not sure if I did everything right with `ch-> to_free-> buffers`

I think you did everything right!

3. I accidentally discovered a bug that CurlFile has. If an object property zval is converted to reference (`$postname =& $file->postname`), then this file property will not pass validation.
   I added checks like this:

if (Z_TYPE_P (prop) == IS_REFERENCE) {
    prop = Z_REFVAL_P (prop);
}

Not sure if this is correct.

Nice catch! I've backported this fix to the 7.4 branch in 54fa0a6, to make sure older versions get fixed.

nikic · 2021-02-02T09:48:47Z

ext/curl/curl_string_file.stub.php

+    public string $postname = "";
+    public ?string $mime = null;
+
+    public function __construct(string $data, string $postname, ?string $mime = null) {}


Not directly related: @kocsismate I just noticed that

php-src/ext/curl/curl_file.stub.php

Line 17 in 8c00c91

public function __construct(string $filename, ?string $mime_type = null, ?string $posted_filename = null) {}

uses different names in the constructor from the property names. We actually managed to have three different names between $postname, $posted_filename and getPostFilename()...

Irker · 2021-02-03T07:57:02Z

@nikic Thanks for the review!
I tried to accommodate all the notes and updated the patch again.
There is only a question with null-byte truncation in postname and mime

nikic

This looks fine to me. Anyone else have feedback?

Personally I'd probably merge curl_string_file.c into curl_file.c, as the final implementation ended up being rather small.

nikic · 2021-02-11T14:14:36Z

ext/curl/curl_string_file.c

+	ZEND_PARSE_PARAMETERS_END();
+
+	zend_update_property_stringl(curl_CURLStringFile_class, Z_OBJ_P(object), "data", sizeof("data")-1, ZSTR_VAL(data), ZSTR_LEN(data));
+	zend_update_property_string(curl_CURLStringFile_class, Z_OBJ_P(object), "postname", sizeof("postname")-1, ZSTR_VAL(postname));


I think it's okay to stick with the CURLFile behavior here.

Irker · 2021-02-11T20:05:43Z

This looks fine to me. Anyone else have feedback?

Personally I'd probably merge curl_string_file.c into curl_file.c, as the final implementation ended up being rather small.

Thank you! I merged files.

kocsismate · 2021-02-12T09:05:21Z

ext/curl/curl_string_file.stub.php

+<?php
+
+/**
+ * @generate-function-entries


You can now get rid of @generate-function-entries

Got it, thank you.

kocsismate · 2021-02-12T09:13:50Z

ext/curl/interface.c

+
+			prop = zend_read_property(curl_CURLStringFile_class, Z_OBJ_P(current), "postname", sizeof("postname")-1, 0, &rv);
+			ZVAL_DEREF(prop);
+			if (Z_TYPE_P(prop) != IS_STRING) {


As CURLStringFile::$postname is a property of type string, is this condition true only if the property is uninitialized, right? If this is the case, shouldn't we throw an exception that we do in other cases? Or am I missing something?

You're right, no need to call php_error_docref here. The exception is already being thrown inside zend_read_property. But I'm do not know how to process it correctly. Is it correct to do just this? :

prop = zend_read_property(curl_CURLStringFile_class, Z_OBJ_P(current), "postname", sizeof("postname")-1, 0, &rv); ZVAL_DEREF(prop); if (Z_TYPE_P(prop) != IS_STRING) { zend_string_release_ex(string_key, 0); return FAILURE; }

I cannot find a good example in other code =/

That would be fine, or possibly something like this?

prop = ...; if (EG(exception)) { zend_string_release_ex(string_key, 0); return FAILURE; } ZVAL_DEREF(prop); ZEND_ASSERT(Z_TYPE_P(prop) == IS_STRING);

kocsismate · 2021-02-12T09:22:40Z

ext/curl/curl_file.c

+	ZEND_PARSE_PARAMETERS_END();
+
+	zend_update_property_str(curl_CURLStringFile_class, Z_OBJ_P(object), "data", sizeof("data") - 1, data);
+	zend_update_property_string(curl_CURLStringFile_class, Z_OBJ_P(object), "postname", sizeof("postname")-1, ZSTR_VAL(postname));


is there any specific reason you use both zend_update_property_string() and zend_update_property_str(), even though the latter would be sufficient (and wouldn't require a ZSTR_VAL() call)?

As I mentioned before we need to truncate mime and postname to null-terminated strings. But looks like it will be truncated during reading their values. So we can use zend_update_property_str for all parameters.
But I do not know how to provide a default value for mime property for method zend_update_property_str. I can use "zend_string_init" method, but at least I do not know which value for its third option I must use.
Right now I know only this way:

if (mime) { zend_update_property_str(curl_CURLStringFile_class, Z_OBJ_P(object), "mime", sizeof("mime")-1, mime); } else { zend_update_property_string(curl_CURLStringFile_class, Z_OBJ_P(object), "mime", sizeof("mime")-1, "application/octet-stream"); }

You suggested code looks reasonable to me.

For zend_string_init the last parameter should be 0, but in this case it's probably a bit nicer to write it as you did.

Irker · 2021-02-15T13:21:55Z

@kocsismate @nikic, Thanks a lot for the review! I updated the PR again =)

> Added `CURLStringFile`, which can be used to post a file from a string rather > than a file: > ```php > $file = new CURLStringFile($data, 'filename.txt', 'text/plain'); > curl_setopt($curl, CURLOPT_POSTFIELDS, ['file' => $file]); > ``` Includes unit test. Refs: * https://github.com/php/php-src/blob/f67986a9218f4889d9352a87c29337a5b6eaa4bd/UPGRADING#L231-L235 * php/php-src#6456 * php/php-src@e727919

> Added `CURLStringFile`, which can be used to post a file from a string rather than a file Refs: * https://github.com/php/php-src/blob/f67986a9218f4889d9352a87c29337a5b6eaa4bd/UPGRADING#L231-L235 * php/php-src#6456 * php/php-src@e727919 As part of the Fibers RFC/PR, the Reflection extension also received a new class. * https://wiki.php.net/rfc/fibers * php/php-src#6875 * php/php-src@c276c16 Note: I've not listed the `Fiber` and `FiberError` classes as those could be considered covered via the mention of Fibers on the "New Features" page, though happy to update the commit if so desired.

> Added `CURLStringFile`, which can be used to post a file from a string rather than a file Refs: * https://github.com/php/php-src/blob/f67986a9218f4889d9352a87c29337a5b6eaa4bd/UPGRADING#L231-L235 * php/php-src#6456 * php/php-src@e727919 As part of the Fibers RFC/PR, the Reflection extension also received a new class. * https://wiki.php.net/rfc/fibers * php/php-src#6875 * php/php-src@c276c16 Note: I've not listed the `Fiber` and `FiberError` classes as those could be considered covered via the mention of Fibers on the "New Features" page, though happy to update the commit if so desired. Co-authored-by: jrfnl <jrfnl@users.noreply.github.com> Closes GH-1447.

> Added `CURLStringFile`, which can be used to post a file from a string rather than a file Refs: * https://github.com/php/php-src/blob/f67986a9218f4889d9352a87c29337a5b6eaa4bd/UPGRADING#L231-L235 * php/php-src#6456 * php/php-src@e727919 As part of the Fibers RFC/PR, the Reflection extension also received a new class. * https://wiki.php.net/rfc/fibers * php/php-src#6875 * php/php-src@c276c16 Note: I've not listed the `Fiber` and `FiberError` classes as those could be considered covered via the mention of Fibers on the "New Features" page, though happy to update the commit if so desired. Co-authored-by: jrfnl <jrfnl@users.noreply.github.com> Closes phpGH-1447.

dtakken reviewed Nov 30, 2020

View reviewed changes

ext/curl/curl.stub.php Outdated Show resolved Hide resolved

ext/curl/curl_string_file.stub.php Outdated Show resolved Hide resolved

nikic reviewed Jan 25, 2021

View reviewed changes

cmb69 reviewed Jan 25, 2021

View reviewed changes

php-pulls pushed a commit that referenced this pull request Feb 2, 2021

Add missing derefs in CurlFile

54fa0a6

As pointed out on GH-6456.

nikic reviewed Feb 2, 2021

View reviewed changes

nikic reviewed Feb 11, 2021

View reviewed changes

kocsismate reviewed Feb 12, 2021

View reviewed changes

cURL: make possible to send file from buffer string

7695287

nikic approved these changes Feb 15, 2021

View reviewed changes

php-pulls closed this in e727919 Feb 16, 2021

Irker deleted the curl_string_file branch February 16, 2021 09:51

Ayesh mentioned this pull request Feb 16, 2021

PHP 8.1 - CURLStringFile symfony/polyfill#333

Closed

mumumu mentioned this pull request Nov 27, 2021

[PHP 8.1] Document CURLStringFile php/doc-en#1154

Merged

jrfnl mentioned this pull request Mar 9, 2022

PHP 8.1 | NewClasses: account for PHP 8.1 changes PHPCompatibility/PHPCompatibility#1328

Merged

jrfnl mentioned this pull request Mar 11, 2022

PHP 8.1 | MigrationGuide/New classes: add missing classes php/doc-en#1447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cURL: make possible to send file from buffer string #6456

cURL: make possible to send file from buffer string #6456

Irker commented Nov 26, 2020 •

edited

Loading

nikic left a comment

nikic Jan 25, 2021

Irker Jan 29, 2021

Irker Jan 30, 2021

nikic Jan 25, 2021

nikic Jan 25, 2021 •

edited

Loading

cmb69 Jan 25, 2021

Irker commented Jan 30, 2021

nikic Feb 2, 2021

nikic Feb 2, 2021

Irker Feb 2, 2021

nikic Feb 11, 2021

nikic Feb 2, 2021

nikic Feb 2, 2021

nikic commented Feb 2, 2021

nikic Feb 2, 2021

kocsismate Feb 2, 2021

Irker commented Feb 3, 2021

nikic left a comment

nikic Feb 11, 2021

Irker commented Feb 11, 2021

kocsismate Feb 12, 2021

Irker Feb 13, 2021

kocsismate Feb 12, 2021

Irker Feb 13, 2021

nikic Feb 13, 2021

kocsismate Feb 12, 2021 •

edited

Loading

Irker Feb 13, 2021 •

edited

Loading

nikic Feb 15, 2021

Irker commented Feb 15, 2021

cURL: make possible to send file from buffer string #6456

cURL: make possible to send file from buffer string #6456

Conversation

Irker commented Nov 26, 2020 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irker commented Jan 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irker commented Feb 3, 2021

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irker commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Irker Feb 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Irker commented Feb 15, 2021

Irker commented Nov 26, 2020 •

edited

Loading

nikic Jan 25, 2021 •

edited

Loading

kocsismate Feb 12, 2021 •

edited

Loading

Irker Feb 13, 2021 •

edited

Loading