Fix #2822: Incorrect usage of fwrite()
We only used to check (and not always) if the return value of fwrite() is boolean FALSE,
while it is possible that the otherwise returned bytecount is less than the length of
data that we're trying to write. This allowed incomplete writes over network streams
and possibly a few other edge cases.
diff --git a/system/core/Log.php b/system/core/Log.php
index ff3c635..63fef20 100644
--- a/system/core/Log.php
+++ b/system/core/Log.php
@@ -178,7 +178,15 @@
$message .= $level.' - '.date($this->_date_fmt).' --> '.$msg."\n";
flock($fp, LOCK_EX);
- fwrite($fp, $message);
+
+ for ($written = 0, $length = strlen($message); $written < $length; $written += $result)
+ {
+ if (($result = fwrite($fp, substr($message, $written))) === FALSE)
+ {
+ break;
+ }
+ }
+
flock($fp, LOCK_UN);
fclose($fp);
@@ -187,7 +195,7 @@
@chmod($filepath, FILE_WRITE_MODE);
}
- return TRUE;
+ return is_int($result);
}
}
diff --git a/system/core/Output.php b/system/core/Output.php
index 10332f0..a7680b3 100644
--- a/system/core/Output.php
+++ b/system/core/Output.php
@@ -542,17 +542,26 @@
return;
}
- $expire = time() + ($this->cache_expiration * 60);
-
- // Put together our serialized info.
- $cache_info = serialize(array(
- 'expire' => $expire,
- 'headers' => $this->headers
- ));
-
if (flock($fp, LOCK_EX))
{
- fwrite($fp, $cache_info.'ENDCI--->'.$output);
+ $expire = time() + ($this->cache_expiration * 60);
+
+ // Put together our serialized info.
+ $cache_info = serialize(array(
+ 'expire' => $expire,
+ 'headers' => $this->headers
+ ));
+
+ $output = $cache_info.'ENDCI--->'.$output;
+
+ for ($written = 0, $length = strlen($output); $written < $length; $written += $result)
+ {
+ if (($result = fwrite($fp, substr($output, $written))) === FALSE)
+ {
+ break;
+ }
+ }
+
flock($fp, LOCK_UN);
}
else
@@ -560,13 +569,22 @@
log_message('error', 'Unable to secure a file lock for file at: '.$cache_path);
return;
}
+
fclose($fp);
- @chmod($cache_path, FILE_WRITE_MODE);
- log_message('debug', 'Cache file written: '.$cache_path);
+ if (is_int($result))
+ {
+ @chmod($cache_path, FILE_WRITE_MODE);
+ log_message('debug', 'Cache file written: '.$cache_path);
- // Send HTTP cache-control headers to browser to match file cache settings.
- $this->set_cache_header($_SERVER['REQUEST_TIME'], $expire);
+ // Send HTTP cache-control headers to browser to match file cache settings.
+ $this->set_cache_header($_SERVER['REQUEST_TIME'], $expire);
+ }
+ else
+ {
+ @unlink($cache_path);
+ log_message('error', 'Unable to write the complete cache content at: '.$cache_path);
+ }
}
// --------------------------------------------------------------------
diff --git a/system/helpers/file_helper.php b/system/helpers/file_helper.php
index 4b45a62..0587740 100644
--- a/system/helpers/file_helper.php
+++ b/system/helpers/file_helper.php
@@ -79,11 +79,19 @@
}
flock($fp, LOCK_EX);
- fwrite($fp, $data);
+
+ for ($written = 0, $length = strlen($data); $written < $length; $written += $result)
+ {
+ if (($result = fwrite($fp, substr($data, $written))) === FALSE)
+ {
+ break;
+ }
+ }
+
flock($fp, LOCK_UN);
fclose($fp);
- return TRUE;
+ return is_int($result);
}
}
diff --git a/system/libraries/Email.php b/system/libraries/Email.php
index 9487ad4..f4efff8 100644
--- a/system/libraries/Email.php
+++ b/system/libraries/Email.php
@@ -2097,7 +2097,16 @@
*/
protected function _send_data($data)
{
- if (fwrite($this->_smtp_connect, $data.$this->newline) === FALSE)
+ $data .= $this->newline;
+ for ($written = 0, $length = strlen($data); $written < $length; $written += $result)
+ {
+ if (($result = fwrite($this->_smtp_connect, substr($data, $written))) === FALSE)
+ {
+ break;
+ }
+ }
+
+ if ($result === FALSE)
{
$this->_set_error_message('lang:email_smtp_data_failure', $data);
return FALSE;
diff --git a/system/libraries/Xmlrpc.php b/system/libraries/Xmlrpc.php
index 1f93e69..ab907e7 100644
--- a/system/libraries/Xmlrpc.php
+++ b/system/libraries/Xmlrpc.php
@@ -724,7 +724,15 @@
.'Content-Length: '.strlen($msg->payload).$r.$r
.$msg->payload;
- if (fwrite($fp, $op, strlen($op)) === FALSE)
+ for ($written = 0, $length = strlen($op); $written < $length; $written += $result)
+ {
+ if (($result = fwrite($fp, substr($op, $written))) === FALSE)
+ {
+ break;
+ }
+ }
+
+ if ($result === FALSE)
{
error_log($this->xmlrpcstr['http_error']);
return new XML_RPC_Response(0, $this->xmlrpcerr['http_error'], $this->xmlrpcstr['http_error']);
diff --git a/system/libraries/Zip.php b/system/libraries/Zip.php
index 250ee02..b10b0bb 100644
--- a/system/libraries/Zip.php
+++ b/system/libraries/Zip.php
@@ -403,11 +403,19 @@
}
flock($fp, LOCK_EX);
- fwrite($fp, $this->get_zip());
+
+ for ($written = 0, $data = $this->get_zip(), $length = strlen($data); $written < $length; $written += $result)
+ {
+ if (($result = fwrite($fp, substr($data, $written))) === FALSE)
+ {
+ break;
+ }
+ }
+
flock($fp, LOCK_UN);
fclose($fp);
- return TRUE;
+ return is_int($result);
}
// --------------------------------------------------------------------
diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst
index 7737852..a1dc3c1 100644
--- a/user_guide_src/source/changelog.rst
+++ b/user_guide_src/source/changelog.rst
@@ -669,6 +669,7 @@
- Fixed a bug (#346) - with ``$config['global_xss_filtering']`` turned on, the ``$_GET``, ``$_POST``, ``$_COOKIE`` and ``$_SERVER`` superglobals were overwritten during initialization time, resulting in XSS filtering being either performed twice or there was no possible way to get the original data, even though options for this do exist.
- Fixed an edge case (#555) - incorrect browser version was reported for Opera 10+ due to a non-standard user-agent string.
- Fixed a bug (#133) - :doc:`Text Helper <helpers/text_helper>` :func:`ascii_to_entities()` stripped the last character if it happens to be in the extended ASCII group.
+- Fixed a bug (#2822) - ``fwrite()`` was used incorrectly throughout the whole framework, allowing incomplete writes when writing to a network stream and possibly a few other edge cases.
Version 2.1.4
=============