patches/28.get_xml_list_leak.patch

Change 550677556 by sungyc@sungyc:fig-export-icing-153-change-391:5978:citc on 2023/07/24 14:40:58

	[hunspell] Fix get_xml_list memory leak
	
	## Test plan
	```
	sso_client -location 'https://clusterfuzz.corp.google.com/testcase-detail/download-testcase?id=5174280127905792' > /tmp/testcase-5174280127905792 && \
	blaze --blazerc=/dev/null test -c opt --config=asan-fuzzer --test_strategy=local --test_sharding_strategy=disabled \
	  --test_env=ENABLE_BLAZE_TEST_FUZZING=1  --test_arg=-runs=100 --test_arg=/tmp/testcase-5174280127905792 \
	  //third_party/hunspell/fuzzers:suggestions_fuzzer
	```
	
	## Description
	It is a special case that causes memory leak. See code comments for explanation.
	
	PRESUBMIT=passed
	BUG=280374372
	R=mghiware
	CC=adorokhine
	APPROVED=mghiware
	REQUIRED_REVIEW=1
	DELTA=35 (30 added, 0 deleted, 5 changed)
	DELTA_BY_EXTENSION=cxx=35
	OCL=550638291
	FIG_CHANGESET=fd93b8de505b51c42e24a7a4711af801f1827adc
	FIG_WORKSPACE=sungyc/153:icing
	MARKDOWN=true

Affected files ...

... //depot//src/hunspell/hunspell.cxx#8 edit

==== //depot//src/hunspell/hunspell.cxx#7 - /google/src/files/550677556/depot//src/hunspell/hunspell.cxx ====
--- /google/src/files/205133714/depot//src/hunspell/hunspell.cxx	2018-07-18 17:06:49.000000000 -0400
+++ /google/src/files/550677556/depot//src/hunspell/hunspell.cxx	2023-07-24 17:40:58.000000000 -0400
@@ -1709,22 +1709,52 @@
 }
 
 int Hunspell::get_xml_list(char ***slst, char * list, const char * tag) {
-    int n = 0;
+    // - Allocates char* array for *slst with length = (# of tags in list).
+    // - Returns n where n = (# of successfully allocated and parsed xml char
+    //   arrays).
+    // - Only (*slst)[0] to (*slst)[n - 1] have valid parsed xml char arrays and
+    //   (*slst)[n] to (*slst)[slst_len - 1] are NULL.
+    *slst = NULL;
+
+    int slst_len = 0;
     char * p;
     if (!list) return 0;
-    for (p = list; (p = strstr(p, tag)); p++) n++;
-    if (n == 0) return 0;
-    *slst = (char **) malloc(sizeof(char *) * n);
+    for (p = list; (p = strstr(p, tag)); p++) slst_len++;
+    if (slst_len == 0) return 0;
+    *slst = (char **) malloc(sizeof(char *) * slst_len);
     if (!*slst) return 0;
+    memset(*slst, 0, sizeof(char *) * slst_len);
+
+    int n;
     for (p = list, n = 0; (p = strstr(p, tag)); p++, n++) {
         int l = strlen(p);
         (*slst)[n] = (char *) malloc(l + 1);
-        if (!(*slst)[n]) return n;
+        if (!(*slst)[n]) {
+          break;
+        }
         if (!get_xml_par((*slst)[n], p + strlen(tag) - 1, l)) {
             free((*slst)[n]);
+            (*slst)[n] = NULL;
             break;
         }
     }
+    // Returns n here so the caller will only use (*slst)[0] to (*slst)[n - 1].
+    // But we have to handle a special case when n == 0 and slst_len > 0:
+    // - The caller eventually calls the util method `freelist()` with the
+    //   returned n, to free the inner char arrays from 0 to n - 1 and the
+    //   entire char* array.
+    // - However, `freelist()` will not free anything when n is 0 and cause
+    //   memory leak for outer char* array (i.e. *slst).
+    // - We could have returned slst_len instead to make `freelist()` work, but
+    //   it changes the original logic because the caller may consequently
+    //   access (*slst)[n] to (*slst)[slst_len - 1] incorrectly.
+    // - Also when n == 0, it means there is no inner parsed xml char array for
+    //   caller to use, so it is correct to make *slst NULL.
+    // - Therefore, let's free *slst properly here.
+    if (n == 0) {
+      free(*slst);
+      *slst = NULL;
+    }
     return n;
 }