From b3d13a000c6fd58d91bdf15fa3abdcc4d3546eff Mon Sep 17 00:00:00 2001 From: Adrien Destugues Date: Sat, 19 Oct 2013 23:31:57 +0200 Subject: [PATCH] Network Kit: Coverity scan review and fixes CID 1108353, 1108335: memory leak. CID 610473: unused variable. CID 1108446, 1108433, 1108432, 1108419, 1108400, 991710, 991713, 991712, 610098, 610097, 610096, 610095: uninitialized field CID 1108421: unused field Change the ownership of the result for Url/HttpRequests. The request now owns its result and you either access it by reference while the request is live, or copy it to keep it after the request destruction. To help with that, get BUrlResult copy constructor and assignment operator to work. Performance issue: copying the BUrlResult also copies the underlying BMallocIO data. This should be shared between the BUrlResult objects to make the copy lighter. The case of BUrlSynchronousRequest is now particularly inefficient, with at least 2 copies needed to get at the result. --- headers/os/net/HttpRequest.h | 2 +- headers/os/net/Url.h | 2 -- headers/os/net/UrlRequest.h | 5 ++--- src/kits/network/libnetapi/HttpForm.cpp | 1 + src/kits/network/libnetapi/HttpRequest.cpp | 7 +++---- src/kits/network/libnetapi/NetworkCookie.cpp | 2 ++ src/kits/network/libnetapi/NetworkCookieJar.cpp | 6 ++---- src/kits/network/libnetapi/Url.cpp | 3 +++ src/kits/network/libnetapi/UrlProtocolRoster.cpp | 6 ++---- src/kits/network/libnetapi/UrlRequest.cpp | 9 +++++---- src/kits/network/libnetapi/UrlResult.cpp | 8 ++++++-- src/kits/network/libnetapi/UrlSynchronousRequest.cpp | 4 +++- 12 files changed, 30 insertions(+), 25 deletions(-) diff --git a/headers/os/net/HttpRequest.h b/headers/os/net/HttpRequest.h index 2ed4a1e4cd..b55ce59a52 100644 --- a/headers/os/net/HttpRequest.h +++ b/headers/os/net/HttpRequest.h @@ -23,7 +23,7 @@ class BAbstractSocket; class BHttpRequest : public BUrlRequest { public: BHttpRequest(const BUrl& url, - BUrlResult& result, bool ssl = false, + bool ssl = false, const char *protocolName = "HTTP", BUrlProtocolListener* listener = NULL, BUrlContext* context = NULL); diff --git a/headers/os/net/Url.h b/headers/os/net/Url.h index 4d08105da1..3b8a5c6d00 100644 --- a/headers/os/net/Url.h +++ b/headers/os/net/Url.h @@ -118,8 +118,6 @@ private: mutable bool fAuthorityValid : 1; mutable bool fUserInfoValid : 1; - bool fBasicUri : 1; - bool fHasProtocol : 1; bool fHasUserName : 1; bool fHasPassword : 1; diff --git a/headers/os/net/UrlRequest.h b/headers/os/net/UrlRequest.h index 82b2dbcc6d..d0b2eb741c 100644 --- a/headers/os/net/UrlRequest.h +++ b/headers/os/net/UrlRequest.h @@ -18,7 +18,6 @@ public: BUrlRequest(const BUrl& url, BUrlProtocolListener* listener, BUrlContext* context, - BUrlResult& result, const char* threadName, const char* protocolName); virtual ~BUrlRequest(); @@ -37,7 +36,7 @@ public: // URL protocol parameters access const BUrl& Url() const; - BUrlResult& Result() const; + const BUrlResult& Result() const; BUrlContext* Context() const; BUrlProtocolListener* Listener() const; const BString& Protocol() const; @@ -63,7 +62,7 @@ protected: protected: BUrl fUrl; - BUrlResult& fResult; + BUrlResult fResult; BUrlContext* fContext; BUrlProtocolListener* fListener; diff --git a/src/kits/network/libnetapi/HttpForm.cpp b/src/kits/network/libnetapi/HttpForm.cpp index 19722eb9a0..1fdc971dd1 100644 --- a/src/kits/network/libnetapi/HttpForm.cpp +++ b/src/kits/network/libnetapi/HttpForm.cpp @@ -704,6 +704,7 @@ BHttpForm::_GetMultipartHeader(const BHttpFormData* element) const BHttpForm::Iterator::Iterator(BHttpForm* form) + : fElement(NULL) { fForm = form; fStdIterator = form->fFields.begin(); diff --git a/src/kits/network/libnetapi/HttpRequest.cpp b/src/kits/network/libnetapi/HttpRequest.cpp index d1ad274e78..2e3e0bda8f 100644 --- a/src/kits/network/libnetapi/HttpRequest.cpp +++ b/src/kits/network/libnetapi/HttpRequest.cpp @@ -28,11 +28,10 @@ static const char* kHttpProtocolThreadStrStatus[ }; -BHttpRequest::BHttpRequest(const BUrl& url, BUrlResult& result, bool ssl, - const char* protocolName, BUrlProtocolListener* listener, - BUrlContext* context) +BHttpRequest::BHttpRequest(const BUrl& url, bool ssl, const char* protocolName, + BUrlProtocolListener* listener, BUrlContext* context) : - BUrlRequest(url, listener, context, result, "BUrlProtocol.HTTP", protocolName), + BUrlRequest(url, listener, context, "BUrlProtocol.HTTP", protocolName), fSocket(NULL), fSSL(ssl), fRequestMethod(B_HTTP_GET), diff --git a/src/kits/network/libnetapi/NetworkCookie.cpp b/src/kits/network/libnetapi/NetworkCookie.cpp index 6a3911d984..346f572c89 100644 --- a/src/kits/network/libnetapi/NetworkCookie.cpp +++ b/src/kits/network/libnetapi/NetworkCookie.cpp @@ -631,6 +631,8 @@ BNetworkCookie::operator!=(const BNetworkCookie& other) void BNetworkCookie::_Reset() { + fInitStatus = false; + fName.Truncate(0); fValue.Truncate(0); fDomain.Truncate(0); diff --git a/src/kits/network/libnetapi/NetworkCookieJar.cpp b/src/kits/network/libnetapi/NetworkCookieJar.cpp index 2a99a0f908..03c894cc1b 100644 --- a/src/kits/network/libnetapi/NetworkCookieJar.cpp +++ b/src/kits/network/libnetapi/NetworkCookieJar.cpp @@ -59,7 +59,7 @@ BNetworkCookieJar::BNetworkCookieJar(BMessage* archive) if (heapCookie == NULL) break; - if (!AddCookie(heapCookie)) { + if (AddCookie(heapCookie) != B_OK) { delete heapCookie; continue; } @@ -69,9 +69,7 @@ BNetworkCookieJar::BNetworkCookieJar(BMessage* archive) BNetworkCookieJar::~BNetworkCookieJar() { - BNetworkCookie* cookiePtr; - - for (Iterator it = GetIterator(); (cookiePtr = it.Next()) != NULL;) + for (Iterator it = GetIterator(); it.Next() != NULL;) delete it.Remove(); } diff --git a/src/kits/network/libnetapi/Url.cpp b/src/kits/network/libnetapi/Url.cpp index 0a76705383..a20fc0eae0 100644 --- a/src/kits/network/libnetapi/Url.cpp +++ b/src/kits/network/libnetapi/Url.cpp @@ -52,6 +52,8 @@ BUrl::BUrl(BMessage* archive) if (archive->FindString(kArchivedUrl, &url) == B_OK) SetUrlString(url); + else + _ResetFields(); } @@ -139,6 +141,7 @@ BUrl::BUrl() fRequest(), fHasAuthority(false) { + _ResetFields(); } diff --git a/src/kits/network/libnetapi/UrlProtocolRoster.cpp b/src/kits/network/libnetapi/UrlProtocolRoster.cpp index e9bc9518fc..2e2a6c917f 100644 --- a/src/kits/network/libnetapi/UrlProtocolRoster.cpp +++ b/src/kits/network/libnetapi/UrlProtocolRoster.cpp @@ -20,14 +20,12 @@ BUrlProtocolRoster::MakeRequest(const BUrl& url, BUrlProtocolListener* listener, BUrlContext* context) { - BUrlResult* result = new BUrlResult(url); - // FIXME this is leaked // TODO: instanciate the correct BUrlProtocol using add-on interface if (url.Protocol() == "http") { - return new(std::nothrow) BHttpRequest(url, *result, false, "HTTP", listener, + return new(std::nothrow) BHttpRequest(url, false, "HTTP", listener, context); } else if (url.Protocol() == "https") { - return new(std::nothrow) BHttpRequest(url, *result, true, "HTTPS", listener, + return new(std::nothrow) BHttpRequest(url, true, "HTTPS", listener, context); } diff --git a/src/kits/network/libnetapi/UrlRequest.cpp b/src/kits/network/libnetapi/UrlRequest.cpp index 95721add85..de51565979 100644 --- a/src/kits/network/libnetapi/UrlRequest.cpp +++ b/src/kits/network/libnetapi/UrlRequest.cpp @@ -28,15 +28,15 @@ static const char* kProtocolThreadStrStatus[B_PROT_THREAD_STATUS__END+1] BUrlRequest::BUrlRequest(const BUrl& url, BUrlProtocolListener* listener, - BUrlContext* context, BUrlResult& result, const char* threadName, - const char* protocolName) + BUrlContext* context, const char* threadName, const char* protocolName) : fUrl(url), - fResult(result), + fResult(url), fContext(context), fListener(listener), fQuit(false), fRunning(false), + fThreadStatus(B_PROT_PAUSED), fThreadId(0), fThreadName(threadName), fProtocol(protocolName) @@ -46,6 +46,7 @@ BUrlRequest::BUrlRequest(const BUrl& url, BUrlProtocolListener* listener, BUrlRequest::~BUrlRequest() { + Stop(); } @@ -168,7 +169,7 @@ BUrlRequest::Url() const } -BUrlResult& +const BUrlResult& BUrlRequest::Result() const { return fResult; diff --git a/src/kits/network/libnetapi/UrlResult.cpp b/src/kits/network/libnetapi/UrlResult.cpp index 260503b58e..bdc20a45aa 100644 --- a/src/kits/network/libnetapi/UrlResult.cpp +++ b/src/kits/network/libnetapi/UrlResult.cpp @@ -18,7 +18,8 @@ BUrlResult::BUrlResult(const BUrl& url) : fUrl(url), fRawData(), - fHeaders() + fHeaders(), + fStatusCode(0) { } @@ -101,7 +102,10 @@ BUrlResult::operator=(const BUrlResult& other) fHeaders = other.fHeaders; fRawData.SetSize(other.fRawData.BufferLength()); - fRawData.WriteAt(0, fRawData.Buffer(), fRawData.BufferLength()); + fRawData.WriteAt(0, other.fRawData.Buffer(), other.fRawData.BufferLength()); + // FIXME this makes a copy of the data, it would be better to share it + + fStatusCode = other.fStatusCode; return *this; } diff --git a/src/kits/network/libnetapi/UrlSynchronousRequest.cpp b/src/kits/network/libnetapi/UrlSynchronousRequest.cpp index b23e092e4f..54437ee451 100644 --- a/src/kits/network/libnetapi/UrlSynchronousRequest.cpp +++ b/src/kits/network/libnetapi/UrlSynchronousRequest.cpp @@ -15,7 +15,7 @@ BUrlSynchronousRequest::BUrlSynchronousRequest(BUrlRequest& request) : - BUrlRequest(request.Url(), this, request.Context(), request.Result(), + BUrlRequest(request.Url(), NULL, request.Context(), "BUrlSynchronousRequest", request.Protocol()), fRequestComplete(false), fWrappedRequest(request) @@ -45,6 +45,8 @@ BUrlSynchronousRequest::WaitUntilCompletion() while (!fRequestComplete) snooze(10000); + fResult = fWrappedRequest.Result(); + return B_OK; }