fix: destruction of HttpClientJson while resetting the reader resulte…

…d in memory corruption (philips-software#404) * fix: destruction of HttpClientJson while resetting the reader resulted in memory corruption * services/network/test/TestHttpClientJson: Fix expectation
gabrielfrasantos · Sep 2, 2023 · ed8a8a0 · ed8a8a0
1 parent 4ac2c8c
commit ed8a8a0
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 1 deletion.
diff --git a/services/network/HttpClientJson.cpp b/services/network/HttpClientJson.cpp
@@ -70,10 +70,14 @@ namespace services
         while (!destructed && jsonParser != infra::none && !stream.Empty())
             (*jsonParser)->Feed(infra::ByteRangeAsString(stream.ContiguousRange()));
 
+        // If this object is already destructed, readerPtr cannot be touched anymore
         if (!destructed)
         {
             readerPtr = nullptr;
-            destructedIndication = nullptr;
+
+            // Destroying the reader can lead to destruction, so check again
+            if (!destructed)
+                destructedIndication = nullptr;
         }
     }
 

diff --git a/services/network/test/TestHttpClientJson.cpp b/services/network/test/TestHttpClientJson.cpp
@@ -164,3 +164,48 @@ TEST_F(HttpClientJsonTest, ContentError_during_parsing_closes_connection)
     EXPECT_CALL(httpClient, CloseConnection());
     httpClient.Observer().BodyAvailable(infra::UnOwnedSharedPtr(reader));
 }
+
+TEST_F(HttpClientJsonTest, close_while_BodyAvailable)
+{
+    EXPECT_CALL(httpClient, Get("/path", testing::_));
+    EXPECT_CALL(controller, Headers()).WillOnce(testing::Return(headersIn));
+    EXPECT_CALL(controller, TopJsonObjectVisitor()).WillOnce(testing::ReturnRef(jsonObjectVisitor));
+    httpClientObserverFactory->ConnectionEstablished([this](infra::SharedPtr<services::HttpClientObserver> client)
+        {
+            httpClient.Attach(client);
+        });
+
+    testing::StrictMock<infra::StreamReaderMock> reader;
+    EXPECT_CALL(reader, Empty()).WillOnce(testing::Return(false));
+    EXPECT_CALL(reader, ExtractContiguousRange(testing::_)).WillOnce(testing::Return(infra::MakeStringByteRange(R"({ })")));
+    EXPECT_CALL(jsonObjectVisitor, Close()).WillOnce(testing::Invoke([this]()
+        {
+            EXPECT_CALL(controller, Error(testing::_));
+            httpClient.Detach();
+            infra::ReConstruct(controller, url, services::HttpClientJson::ConnectionInfo{ jsonParserCreator, 443, httpClientConnector }, services::noAutoConnect);
+        }));
+    httpClient.Observer().BodyAvailable(infra::UnOwnedSharedPtr(reader));
+}
+
+TEST_F(HttpClientJsonTest, close_upon_destructing_reader_in_BodyAvailable)
+{
+    EXPECT_CALL(httpClient, Get("/path", testing::_));
+    EXPECT_CALL(controller, Headers()).WillOnce(testing::Return(headersIn));
+    EXPECT_CALL(controller, TopJsonObjectVisitor()).WillOnce(testing::ReturnRef(jsonObjectVisitor));
+    httpClientObserverFactory->ConnectionEstablished([this](infra::SharedPtr<services::HttpClientObserver> client)
+        {
+            httpClient.Attach(client);
+        });
+
+    testing::StrictMock<infra::StreamReaderMock> reader;
+    EXPECT_CALL(reader, Empty()).WillOnce(testing::Return(false)).WillOnce(testing::Return(true));
+    EXPECT_CALL(reader, ExtractContiguousRange(testing::_)).WillOnce(testing::Return(infra::MakeStringByteRange(R"({ })")));
+    EXPECT_CALL(jsonObjectVisitor, Close());
+    infra::AccessedBySharedPtr access([this]()
+        {
+            EXPECT_CALL(controller, Error(testing::_));
+            httpClient.Detach();
+            infra::ReConstruct(controller, url, services::HttpClientJson::ConnectionInfo{ jsonParserCreator, 443, httpClientConnector }, services::noAutoConnect);
+        });
+    httpClient.Observer().BodyAvailable(access.MakeShared(reader));
+}