Chwala Bug when using external webdav (fopen is a handle not a variable) !
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	freakenfurter
	Nov 23 2017, 2:10 PM

Description

Problem:
Files uploaded via roundcube and via save to cloud are created but have the size of 0 bytes.
The code seems incomplete since a filehandle actually is a handle not a variable.
So line 334 will cause a SabreDav Exception which isn't handled right now.
My fix works when reading the handle and putting it into a variable:

--- webdav_file_storage.php	2017-11-23 14:05:00.000000000 +0100
+++ webdav_file_storage.php.new	2017-11-23 14:08:15.696683098 +0100
@@ -334,7 +334,12 @@
         $this->init();

         if ($file['path']) {
-            $data = fopen($file['path'], 'r');
+	    $myfile = fopen($file['path'], 'r');
+	    if ( $myfile === false ) {
+		throw new Exception("Storage error. Cannot open temporary file " . $file['path'], file_storage::ERROR);
+	    }
+	    $data = fread($myfile,filesize($file['path']));
+	    fclose($myfile);
         }
         else {
             // Resource or data
@@ -362,7 +367,12 @@
         $this->init();

         if ($file['path']) {
-            $data = fopen($file['path'], 'r');
+            $myfile = fopen($file['path'], 'r');
+	    if ( $myfile === false ) {
+		throw new Exception("Storage error. Cannot open temporary file " . $file['path'], file_storage::ERROR);
+	    }
+	    $data = fread($myfile,filesize($file['path']));
+	    fclose($myfile);
         }
         else {
             //Resource or data

Tested it and of course it works ;-)

Best regards

Franz

Details

Ticket Type: Task

Related Objects

Mentioned Here: rC57e061251ee2: DAV/Client does not do that, we have to do this.

Event Timeline

freakenfurter created this task.Nov 23 2017, 2:10 PM

Sabre (and cURL) supports the body as a resource. So, the bug is somewhere else.

I din't know what you mean.
You use the SABRE Client DAV library which takes care of the workflow.
The first requests consists of a PUT request which then creates the file (0 bytes).
The 100-Continue header will be EXPECTed.
Then the data will we sent to the server by reusing the connection (keep alive) and otherwise will generate an error ( e.g 499).
It's clear that there's a bug in your code because it's a fact that you cannot assign a file handle as variable.
And obviously you don't catch the exception raised by SABRE.

Your code snip as a testprogram:

<?php
// try SabreDAV installed as described in README
if (file_exists(__DIR__ . '/../../ext/SabreDAV/vendor/autoload.php')) {
    require __DIR__ . '/../../ext/SabreDAV/vendor/autoload.php';
}
// fallback to System-installed package
else {
    require 'sabre21/Sabre/autoload.php';
}

use Sabre\DAV\Client;

    $data = fopen("testpic.jpg", "r");

$settings = array(
    'baseUri' => 'https://nextcloud/remote.php/webdav/',
    'userName' => 'username',
    'password' => 'password',
);

$client = new Client($settings);
$response = $client->request('PUT', 'https://nextcloud/remote.php/webdav/Documents/testpic.png', $data);

Will cause this exception: (SABRE expects a variable not a filehandle).

PHP Fatal error:  Uncaught Sabre\HTTP\ClientException: necessary data rewind wasn't possible in /usr/share/chwala/lib/ext/SabreDAV/vendor/sabre/http/lib/Client.php:342
Stack trace:
#0 /usr/share/chwala/lib/ext/SabreDAV/vendor/sabre/http/lib/Client.php(101): Sabre\HTTP\Client->doRequest(Object(Sabre\HTTP\Request))
#1 /usr/share/chwala/lib/ext/SabreDAV/vendor/sabre/dav/lib/DAV/Client.php(389): Sabre\HTTP\Client->send(Object(Sabre\HTTP\Request))
#2 /usr/share/chwala/lib/drivers/webdav/client-bad.php(22): Sabre\DAV\Client->request('PUT', 'https://nextcl...', Resource id #9)
#3 {main}
  thrown in /usr/share/chwala/lib/ext/SabreDAV/vendor/sabre/http/lib/Client.php on line 342

My working testprogram:

<?php
// try SabreDAV installed as described in README
if (file_exists(__DIR__ . '/../../ext/SabreDAV/vendor/autoload.php')) {
    require __DIR__ . '/../../ext/SabreDAV/vendor/autoload.php';
}
// fallback to System-installed package
else {
    require 'sabre21/Sabre/autoload.php';
}

use Sabre\DAV\Client;

$myfile = fopen("webdav.png", "r") or die("Unable to open file!");
$data = fread($myfile,filesize('webdav.png'));
fclose($myfile);

$settings = array(
    'baseUri' => 'https://nextcloud/remote.php/webdav/',
    'userName' => 'username',
    'password' => 'password',
);

$client = new Client($settings);
$response = $client->request('PUT', 'https://nextcloud/remote.php/webdav/Documents/webdav.png', $data);

Of course, no exception will be generated and the create function works.
I really think you should fix that bug.

Best regards
Franz

I'm telling you Sabre supports file handles (https://github.com/sabre-io/http/blob/master/lib/Client.php#L397). Problem is that for some reason the handle is not seekable, which is a requirement from curl. From what I know rewind is needed in case of connection redirect or interruption, but anyway I don't see why it couldn't rewind.

Do you see the same exception in Chwala log?

Problem with your patch is that you load the whole file into memory, while it should not be needed. What PHP version you're using?

Hi Alexander,
plain jessie php PHP 5.6.30-0+deb8u1 with no modifications.
Also i checked curl C sources. It will read from one file socket and write directly to the remote socket.
So you're right. I can recall have owncloud years before it didn't work either.
Don't know if it was Sabre 2.1.6 back then.
Anyhow, i found this post which is dated from 2013. Nevertheless the options doesn't exist neither in the current CURL version nur in the php module.
Link:
https://github.com/aws/aws-sdk-php/issues/140

Best regards
Franz

I didn't read curl sources, but maybe Sabre/HTTP/Client should define CURLOPT_READFUNCTION.

Hi again,
i now scanned the latest php 7 source and the tests are very interesting.
E.g:
curl_setopt_CURLOPT_READFUNCTION.phpt

--SKIPIF--
<?php
if (!extension_loaded("curl")) print "skip cURL extension not loaded";
?>
--FILE--
<?php
function custom_readfunction($oCurl, $hReadHandle, $iMaxOut)
{
  $sData = fread($hReadHandle,$iMaxOut-10); # -10 to have space to add "custom:"
  if (!empty($sData))
  {
    $sData = "custom:".$sData;
  }
  return $sData;
}

So they read the whole file using fread and return the data.
IMHO, they don't support reading from a file handle anymore.
The C source reads it from the handle and send it to the connected TCP socket.
Perhaps the servers now have enough memory and they dropped it ?
Rgds.
Franz

pasik subscribed.Nov 25 2017, 2:25 PM

Hi Alexander,
i now solved the problem by reading the C-Source.
f you use http PUT with curl then you have to specify the Content-Length header. Otherwise it is automatically handled by the curl for both GET and POST.
This has to be done by Sabre which it doesn't in the particular version 2.1.6.
So, the Content-Header: will be defined using 0 bytes. That's it. you only have to specify the content-lenght header via setopt and it works.
Since the obsys kolab mirror is down right now, i cannot create a detailed diff. (removed the source files by accident and don't know if git is in sync with the package files).
This explains why there's no error in the logfile visible. I missed the authType in my testscript and so the error appeared ;-)
Sorry for that.
Nevertheless, it works now.

--- webdav_file_storage.php.ori	2017-11-27 12:04:13.664027166 +0100
+++ webdav_file_storage.php.new	2017-11-27 12:05:15.281488406 +0100
@@ -334,14 +334,19 @@

         if ($file['path']) {
 	    $data = fopen($file['path'], 'r');
+	    $this->client->addCurlSetting(CURLOPT_INFILESIZE, filesize($file['path']));
         }
         else {
             // Resource or data
             $data = $file['content'];
+            $this->client->addCurlSetting(CURLOPT_HTTPHEADER, array(
+		'Content-Length: ' . strlen($data))
+	    );
         }

         $file_name = $this->encode_path($file_name);
 	$response  = $this->client->request('PUT', $file_name, $data);
+	gettype ($data) === 'resource' ? fclose($data) : null;

         if ($response['statusCode'] != 201) {
             throw new Exception("Storage error. " . $response['body'], file_storage::ERROR);
@@ -361,15 +366,20 @@
 	$this->init();

         if ($file['path']) {
+	    $this->client->addCurlSetting(CURLOPT_INFILESIZE, filesize($file['path']));
 	    $data = fopen($file['path'], 'r');
         }
         else {
             // Resource or data
             $data = $file['content'];
+            $this->client->addCurlSetting(CURLOPT_HTTPHEADER, array(
+		'Content-Length: ' . strlen($data))
+	    );
         }

         $file_name = $this->encode_path($file_name);
 	$response  = $this->client->request('PUT', $file_name, $data);
+	gettype ($data) === 'resource' ? fclose($data) : null;

         if ($response['statusCode'] != 204) {
             throw new Exception("Storage error. " . $response['body'], file_storage::ERROR);

Best regards
Franz Skale

Could we just set content-length header using 4th argument of the client->request() method? I mean use the header for both operation modes, i.e. for file as a stream and as a string. These curl settings look very hackish.

I don't see nothing like that in Sabre most recent code. This is really strange no one hit this issue before. Thank you for your efforts.

Headers would be possible but then the header: Transfer-Encoding: chunked will be added and fails. (nginx latest doesn't work).
Stated here, http://sabre.io/dav/0bytes/ it should work but it don't.
So, when using fpm either nginx or apaches fastcgi it won't work as expected.
Since my "hacky" workaround works i'll leave it that way.
To be compatible it should be mentioned in the docs.
Task can be closed.

Rgds.
Franz

Fixed in 57e061251ee.

Chwala Bug when using external webdav (fopen is a handle not a variable) !Closed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

Chwala Bug when using external webdav (fopen is a handle not a variable) !
Closed, ResolvedPublic
Actions