Merged Pull Request '#60 fix/iterate-empty-evidence->version/4.5: BUG…

…: iterating over empty evidence could segfault' BUG: iterating over empty evidence could segfault
51Degrees · Nov 4, 2024 · 5f88ed2 · 5f88ed2
2 parents 66f6c56 + dc7edca
commit 5f88ed2
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 10 deletions.
diff --git a/array.h b/array.h
@@ -69,12 +69,12 @@ typedef struct fiftyone_degrees_array_##t##_t { \
 #define FIFTYONE_DEGREES_ARRAY_CREATE(t, i, c) \
 i = (t##Array*)fiftyoneDegreesMalloc(FIFTYONE_DEGREES_ARRAY_SIZE(t,c)); \
 if (i != NULL) { \
-i->items = (t*)(i + 1); \
+i->items = c ? (t*)(i + 1) : NULL; \
 i->count = 0; \
 i->capacity = c; \
 }
 
 /**
  * @}
  */ 
-#endif
+#endif
diff --git a/evidence.c b/evidence.c
@@ -94,14 +94,16 @@ static uint32_t evidenceIterate(
 
 		// Check the current evidence item and call back if the right prefix
 		// after parsing the pair if not done so already.
-		pair = &evidence->items[index++];
-		if ((pair->prefix & prefixes) == pair->prefix) {
-			if (pair->parsedValue == NULL) {
-				parsePair(pair);
-			}
-			cont = callback(state, pair);
-			iterations++;
-		}
+        if (index < evidence->count) {
+            pair = &evidence->items[index++];
+            if ((pair->prefix & prefixes) == pair->prefix) {
+                if (pair->parsedValue == NULL) {
+                    parsePair(pair);
+                }
+                cont = callback(state, pair);
+                iterations++;
+            }
+        }
 
 		// Check if the next evidence array needs to be moved to.
 		if (index >= evidence->count) {

diff --git a/tests/EvidenceTests.cpp b/tests/EvidenceTests.cpp
@@ -23,6 +23,9 @@
 #include "pch.h"
 #include "EvidenceTests.hpp"
 #include "memory.h"
+#include "../EvidenceBase.hpp"
+
+using namespace FiftyoneDegrees::Common;
 
 void assertStringHeaderAdded(
 	fiftyoneDegreesEvidenceKeyValuePair *pair,
@@ -442,3 +445,17 @@ TEST_F(Evidence, freeNullEvidence) {
     fiftyoneDegreesEvidenceKeyValuePairArray *evidence2 = NULL;
     EvidenceFree(evidence2);
 }
+
+TEST_F(Evidence, emptyEvidence) {
+    EvidenceBase emptyEvidence;
+    //this produces an empty array, but items pointer is set to 1 past the end of array structure,
+    //so we do not always segfault if we attempt to iterate over it
+    //address sanitizer always reveals this heap overflow however
+    fiftyoneDegreesEvidenceKeyValuePairArray *emptyEvidenceKVPA = emptyEvidence.get();
+
+    std::vector<std::string> results;
+    auto iterations = EvidenceIterate(emptyEvidenceKVPA, FIFTYONE_DEGREES_EVIDENCE_QUERY,
+     &results, callback1);
+
+    EXPECT_EQ(iterations, 0);
+}