MT#59962 XmlRpcSocket: refactor `nbWrite()`, make it safer

Rework the `nbWrite()` function so, that it gets safe
in terms of:
- operating on data written/data remaining integers
- has protection guards for extra big chunks to write at once

Refactor to use size_t/ssize_t instead of primitive int,
where possible (e.g. bytesWritten).

Let the `nbWrite()` use reference instead of pointer
for the bytes written parameter.

Fixes Coverity Scan report:

     4. tainted_data_return: Called function write(fd, sp, nToWrite),
        and a possible return value may be less than zero.
     5. cast_overflow: An assign that casts to a different type,
        which might trigger an overflow.

     Overflowed integer argument (INTEGER_OVERFLOW)
     13. overflow_sink: nToWrite, which might have overflowed,
         is passed to write(fd, sp, nToWrite).

      7. overflow: The expression nToWrite is considered
         to have possibly overflowed.

Change-Id: I32aa6aae5ef5715d61a91714e62b0a094bc03f21
master
Donat Zenichev 1 week ago
parent 5d1ccbbc86
commit 9a74726171

@ -394,15 +394,15 @@ XmlRpcClient::writeRequest()
XmlRpcUtil::log(5, "XmlRpcClient::writeRequest (attempt %d):\n%s\n", _sendAttempts+1, _request.c_str());
// Try to write the request
if ( ! XmlRpcSocket::nbWrite(this->getfd(), _request, &_bytesWritten, _ssl_ssl)) {
if ( ! XmlRpcSocket::nbWrite(this->getfd(), _request, _bytesWritten, _ssl_ssl)) {
XmlRpcUtil::error("Error in XmlRpcClient::writeRequest: write error (%s).",XmlRpcSocket::getErrorMsg().c_str());
return false;
}
XmlRpcUtil::log(3, "XmlRpcClient::writeRequest: wrote %d of %d bytes.", _bytesWritten, _request.length());
XmlRpcUtil::log(3, "XmlRpcClient::writeRequest: wrote %zd of %d bytes.", _bytesWritten, _request.length());
// Wait for the result
if (_bytesWritten == int(_request.length())) {
if (_bytesWritten == _request.length()) {
_header = "";
_response = "";
_connectionState = READ_HEADER;

@ -123,7 +123,7 @@ namespace XmlRpc {
int _sendAttempts;
// Number of bytes of the request that have been written to the socket so far
int _bytesWritten;
size_t _bytesWritten;
// True if we are currently executing a request. If you want to multithread,
// each thread should have its own client.

@ -182,14 +182,14 @@ XmlRpcServerConnection::writeResponse()
}
// Try to write the response
if ( ! XmlRpcSocket::nbWrite(this->getfd(), _response, &_bytesWritten, _ssl_ssl)) {
if ( ! XmlRpcSocket::nbWrite(this->getfd(), _response, _bytesWritten, _ssl_ssl)) {
XmlRpcUtil::error("XmlRpcServerConnection::writeResponse: write error (%s).",XmlRpcSocket::getErrorMsg().c_str());
return false;
}
XmlRpcUtil::log(3, "XmlRpcServerConnection::writeResponse: wrote %d of %zu bytes.", _bytesWritten, static_cast<size_t>(_response.length()));
XmlRpcUtil::log(3, "XmlRpcServerConnection::writeResponse: wrote %zu of %zu bytes.", _bytesWritten, _response.length());
// Prepare to read the next request
if (_bytesWritten == int(_response.length())) {
if (_bytesWritten == _response.length()) {
_header = "";
_request = "";
_response = "";

@ -72,7 +72,7 @@ namespace XmlRpc {
std::string _response;
//! Number of bytes of the response written so far
int _bytesWritten;
size_t _bytesWritten;
//! Whether to keep the current client connection open for further requests
bool _keepAlive;

@ -35,9 +35,10 @@ extern "C" {
#include <string.h>
#include <strings.h>
using namespace XmlRpc;
#include <limits>
using std::numeric_limits;
using namespace XmlRpc;
#if defined(_WINDOWS)
@ -214,35 +215,51 @@ XmlRpcSocket::nbRead(int fd, std::string& s, bool *eof, SSL* ssl)
}
// Write text to the specified socket. Returns false on error.
bool
XmlRpcSocket::nbWrite(int fd, std::string& s, int *bytesSoFar, SSL* ssl)
/* Write text to the specified socket. Returns false on error. */
bool XmlRpcSocket::nbWrite(int fd, std::string& s, size_t &bytesSoFar, SSL* ssl)
{
int nToWrite = int(s.length()) - *bytesSoFar;
char *sp = const_cast<char*>(s.c_str()) + *bytesSoFar;
/* a guard against overflow */
if (bytesSoFar < 0 || bytesSoFar > s.length()) {
XmlRpcUtil::log(1, "XmlRpcSocket::nbWrite: Invalid bytesSoFar='%zu' for string length='%zu'",
bytesSoFar, s.length());
return false;
}
bool wouldBlock = false;
while ( nToWrite > 0 && ! wouldBlock ) {
#if defined(_WINDOWS)
int n = send(fd, sp, nToWrite, 0);
#else
int n;
if (ssl != (SSL *) NULL) {
n = SSL_write(ssl, sp, nToWrite);
size_t totalLen = s.length();
size_t bytesRemaining = totalLen - static_cast<size_t>(bytesSoFar);
char *sp = const_cast<char*>(s.c_str()) + bytesSoFar;
while (bytesRemaining > 0 && !wouldBlock)
{
ssize_t bytesWritten;
if (ssl != nullptr) {
if (bytesRemaining > static_cast<size_t>(numeric_limits<int>::max())) {
XmlRpcUtil::log(1, "Too much data to be written at once via SSL_write().");
return false;
}
bytesWritten = SSL_write(ssl, sp, static_cast<int>(bytesRemaining));
} else {
n = write(fd, sp, nToWrite);
bytesWritten = write(fd, sp, bytesRemaining);
/* guard */
if (bytesWritten > 0 && static_cast<size_t>(bytesWritten) > bytesRemaining) {
XmlRpcUtil::log(1, "XmlRpcSocket::nbWrite: write() returned more than requested (%zd vs %zu)", bytesWritten, bytesRemaining);
return false;
}
}
#endif
XmlRpcUtil::log(5, "XmlRpcSocket::nbWrite: send/write returned %d.", n);
if (n > 0) {
sp += n;
*bytesSoFar += n;
nToWrite -= n;
XmlRpcUtil::log(5, "XmlRpcSocket::nbWrite: send/write returned '%zd'.", bytesWritten);
if (bytesWritten > 0) {
sp += bytesWritten;
bytesSoFar += bytesWritten;
bytesRemaining -= bytesWritten;
} else if (nonFatalError()) {
wouldBlock = true;
} else {
return false; // Error
return false; /* Error */
}
}
return true;

@ -34,7 +34,7 @@ namespace XmlRpc {
static bool nbRead(int socket, std::string& s, bool *eof, SSL *ssl);
//! Write text to the specified socket. Returns false on error.
static bool nbWrite(int socket, std::string& s, int *bytesSoFar, SSL* ssl);
static bool nbWrite(int socket, std::string& s, size_t &bytesSoFar, SSL* ssl);
// The next four methods are appropriate for servers.

Loading…
Cancel
Save