Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: memory leak in MMDB_open() #356

Merged
merged 2 commits into from
Nov 8, 2024
Merged

fix: memory leak in MMDB_open() #356

merged 2 commits into from
Nov 8, 2024

Conversation

pkillarjun
Copy link
Contributor

While I was fuzzing, I found a total of three memory leaks. This is the patch.

@pkillarjun pkillarjun mentioned this pull request Nov 5, 2024
Merged
@pkillarjun
Copy link
Contributor Author

pkillarjun commented Nov 5, 2024 •
edited
Loading

Reproduce bug-1

$ ./t/fuzz_mmdb leak-populate_languages_metadata-1.bug.zip

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 632479687
INFO: Loaded 1 modules   (2851 inline 8-bit counters): 2851 [0x5d4750, 0x5d5273), 
INFO: Loaded 1 PC tables (2851 PCs): 2851 [0x5d5278,0x5e04a8), 
./t/fuzz_mmdb: Running 1 inputs 1 time(s) each.
Running: leak-populate_languages_metadata-1.bug.zip

=================================================================
==664668==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 4096 byte(s) in 1 object(s) allocated from:
    #0 0x0000004daf3d in calloc (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4daf3d) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #1 0x00000054ddcd in data_pool_new /home/cia/Desktop/libmaxminddb/src/data-pool.c:26:23
    #2 0x00000052f8f8 in MMDB_get_entry_data_list /home/cia/Desktop/libmaxminddb/src/maxminddb.c:1636:36
    #3 0x000000538ee1 in populate_languages_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:733:14
    #4 0x0000005227a3 in read_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:582:14
    #5 0x00000051dfc3 in MMDB_open /home/cia/Desktop/libmaxminddb/src/maxminddb.c:285:14
    #6 0x00000051c88a in LLVMFuzzerTestOneInput /home/cia/Desktop/libmaxminddb/t/fuzz_mmdb.c:30:14
    #7 0x0000004204ba in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4204ba) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #8 0x0000004079d7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4079d7) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #9 0x00000040e016 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x40e016) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #10 0x00000043afe6 in main (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x43afe6) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #11 0x7f4c13811247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #12 0x7f4c1381130a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #13 0x000000401a94 in _start (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x401a94) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)

Indirect leak of 544 byte(s) in 1 object(s) allocated from:
    #0 0x0000004daf3d in calloc (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4daf3d) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #1 0x00000054db11 in data_pool_new /home/cia/Desktop/libmaxminddb/src/data-pool.c:15:36
    #2 0x00000052f8f8 in MMDB_get_entry_data_list /home/cia/Desktop/libmaxminddb/src/maxminddb.c:1636:36
    #3 0x000000538ee1 in populate_languages_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:733:14
    #4 0x0000005227a3 in read_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:582:14
    #5 0x00000051dfc3 in MMDB_open /home/cia/Desktop/libmaxminddb/src/maxminddb.c:285:14
    #6 0x00000051c88a in LLVMFuzzerTestOneInput /home/cia/Desktop/libmaxminddb/t/fuzz_mmdb.c:30:14
    #7 0x0000004204ba in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4204ba) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #8 0x0000004079d7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4079d7) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #9 0x00000040e016 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x40e016) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #10 0x00000043afe6 in main (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x43afe6) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #11 0x7f4c13811247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #12 0x7f4c1381130a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #13 0x000000401a94 in _start (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x401a94) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)

SUMMARY: AddressSanitizer: 4640 byte(s) leaked in 2 allocation(s).

INFO: a leak has been found in the initial corpus.

INFO: to ignore leaks on libFuzzer side use -detect_leaks=0.

Fix-patch

diff --git a/src/maxminddb.c b/src/maxminddb.c
index 55a3ce2..ba2ac1d 100644
--- a/src/maxminddb.c
+++ b/src/maxminddb.c
@@ -732,6 +732,8 @@ static int populate_languages_metadata(MMDB_s *mmdb,
     MMDB_entry_data_list_s *member;
     status = MMDB_get_entry_data_list(&array_start, &member);
     if (MMDB_SUCCESS != status) {
+         if (MMDB_OUT_OF_MEMORY_ERROR != status)
+            MMDB_free_entry_data_list(member);
         return status;
     }

Note: leak-populate_languages_metadata-1.bug.zip is not a .zip file.

leak-populate_languages_metadata-1.bug.zip

@pkillarjun
Copy link
Contributor Author

pkillarjun commented Nov 5, 2024 •
edited
Loading

Reproduce bug

$ ./t/fuzz_mmdb leak-populate_description_metadata-2.bug.zip

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3528603934
INFO: Loaded 1 modules   (2852 inline 8-bit counters): 2852 [0x5d4750, 0x5d5274), 
INFO: Loaded 1 PC tables (2852 PCs): 2852 [0x5d5278,0x5e04b8), 
./t/fuzz_mmdb: Running 1 inputs 1 time(s) each.
Running: leak-populate_description_metadata-2.bug.zip

=================================================================
==689593==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 4096 byte(s) in 1 object(s) allocated from:
    #0 0x0000004daf3d in calloc (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4daf3d) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #1 0x00000054de0d in data_pool_new /home/cia/Desktop/libmaxminddb/src/data-pool.c:26:23
    #2 0x00000052f8f8 in MMDB_get_entry_data_list /home/cia/Desktop/libmaxminddb/src/maxminddb.c:1638:36
    #3 0x00000053b2c1 in populate_description_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:793:14
    #4 0x000000523038 in read_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:619:14
    #5 0x00000051dfc3 in MMDB_open /home/cia/Desktop/libmaxminddb/src/maxminddb.c:285:14
    #6 0x00000051c88a in LLVMFuzzerTestOneInput /home/cia/Desktop/libmaxminddb/t/fuzz_mmdb.c:30:14
    #7 0x0000004204ba in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4204ba) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #8 0x0000004079d7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4079d7) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #9 0x00000040e016 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x40e016) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #10 0x00000043afe6 in main (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x43afe6) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #11 0x7f86671e2247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #12 0x7f86671e230a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #13 0x000000401a94 in _start (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x401a94) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)

Indirect leak of 544 byte(s) in 1 object(s) allocated from:
    #0 0x0000004daf3d in calloc (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4daf3d) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #1 0x00000054db51 in data_pool_new /home/cia/Desktop/libmaxminddb/src/data-pool.c:15:36
    #2 0x00000052f8f8 in MMDB_get_entry_data_list /home/cia/Desktop/libmaxminddb/src/maxminddb.c:1638:36
    #3 0x00000053b2c1 in populate_description_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:793:14
    #4 0x000000523038 in read_metadata /home/cia/Desktop/libmaxminddb/src/maxminddb.c:619:14
    #5 0x00000051dfc3 in MMDB_open /home/cia/Desktop/libmaxminddb/src/maxminddb.c:285:14
    #6 0x00000051c88a in LLVMFuzzerTestOneInput /home/cia/Desktop/libmaxminddb/t/fuzz_mmdb.c:30:14
    #7 0x0000004204ba in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4204ba) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #8 0x0000004079d7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x4079d7) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #9 0x00000040e016 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x40e016) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #10 0x00000043afe6 in main (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x43afe6) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)
    #11 0x7f86671e2247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #12 0x7f86671e230a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #13 0x000000401a94 in _start (/home/cia/Desktop/libmaxminddb/build/t/fuzz_mmdb+0x401a94) (BuildId: 04e1f3887a753f5af66681be13431ac54c0dc78b)

SUMMARY: AddressSanitizer: 4640 byte(s) leaked in 2 allocation(s).

INFO: a leak has been found in the initial corpus.

INFO: to ignore leaks on libFuzzer side use -detect_leaks=0.

Fix-patch

diff --git a/src/maxminddb.c b/src/maxminddb.c
index 55a3ce2..e8474c1 100644
--- a/src/maxminddb.c
+++ b/src/maxminddb.c
@@ -795,6 +795,8 @@ static int populate_description_metadata(MMDB_s *mmdb,
             " status = %d (%s)",
             status,
             MMDB_strerror(status));
+        if (status != MMDB_OUT_OF_MEMORY_ERROR)
+            MMDB_free_entry_data_list(member);
         return status;
     }

Note: leak-populate_description_metadata-2.bug.zip is not a .zip file.

leak-populate_description_metadata-2.bug.zip

@pkillarjun
Copy link
Contributor Author

Reproduce bug

$ ./t/fuzz_mmdb leak-populate_languages_metadata-3.bug.zip

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3963995163
INFO: Loaded 1 modules   (2851 inline 8-bit counters): 2851 [0x5d4750, 0x5d5273), 
INFO: Loaded 1 PC tables (2851 PCs): 2851 [0x5d5278,0x5e04a8), 
./t/fuzz_mmdb: Running 1 inputs 1 time(s) each.
Running: leak-populate_languages_metadata-3.bug.zip

=================================================================
==694658==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 4096 byte(s) in 1 object(s) allocated from:
    #0 0x0000004daf3d in calloc (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x4daf3d) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #1 0x00000054ddcd in data_pool_new /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/data-pool.c:26:23
    #2 0x00000052f8f8 in MMDB_get_entry_data_list /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:1636:36
    #3 0x000000538ee1 in populate_languages_metadata /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:733:14
    #4 0x0000005227a3 in read_metadata /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:582:14
    #5 0x00000051dfc3 in MMDB_open /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:285:14
    #6 0x00000051c88a in LLVMFuzzerTestOneInput /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/t/fuzz_mmdb.c:30:14
    #7 0x0000004204ba in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x4204ba) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #8 0x0000004079d7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x4079d7) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #9 0x00000040e016 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x40e016) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #10 0x00000043afe6 in main (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x43afe6) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #11 0x7fdad97e2247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #12 0x7fdad97e230a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #13 0x000000401a94 in _start (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x401a94) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)

Indirect leak of 544 byte(s) in 1 object(s) allocated from:
    #0 0x0000004daf3d in calloc (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x4daf3d) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #1 0x00000054db11 in data_pool_new /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/data-pool.c:15:36
    #2 0x00000052f8f8 in MMDB_get_entry_data_list /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:1636:36
    #3 0x000000538ee1 in populate_languages_metadata /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:733:14
    #4 0x0000005227a3 in read_metadata /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:582:14
    #5 0x00000051dfc3 in MMDB_open /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/src/maxminddb.c:285:14
    #6 0x00000051c88a in LLVMFuzzerTestOneInput /home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/t/fuzz_mmdb.c:30:14
    #7 0x0000004204ba in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x4204ba) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #8 0x0000004079d7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x4079d7) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #9 0x00000040e016 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x40e016) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #10 0x00000043afe6 in main (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x43afe6) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)
    #11 0x7fdad97e2247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #12 0x7fdad97e230a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 948d229348b56bdcf4e0a96d5ccd8fb175f617f0)
    #13 0x000000401a94 in _start (/home/craft/Desktop/oss-fuzz/maxmind/libmaxminddb.fuzzing/build/t/fuzz_mmdb+0x401a94) (BuildId: 638bb229cb34f10fb11a50ba0ab4cb979181bb30)

SUMMARY: AddressSanitizer: 4640 byte(s) leaked in 2 allocation(s).

INFO: a leak has been found in the initial corpus.

INFO: to ignore leaks on libFuzzer side use -detect_leaks=0.

Fix-patch

diff --git a/src/maxminddb.c b/src/maxminddb.c
index 55a3ce2..d8b4ca4 100644
--- a/src/maxminddb.c
+++ b/src/maxminddb.c
@@ -750,6 +750,7 @@ static int populate_languages_metadata(MMDB_s *mmdb,
     for (uint32_t i = 0; i < array_size; i++) {
         member = member->next;
         if (MMDB_DATA_TYPE_UTF8_STRING != member->entry_data.type) {
+            MMDB_free_entry_data_list(first_member);
             return MMDB_INVALID_METADATA_ERROR;
         }

Note: leak-populate_languages_metadata-3.bug.zip is not a .zip file.
leak-populate_languages_metadata-3.bug.zip

@pkillarjun pkillarjun force-pushed the memory-leak branch 2 times, most recently from b48e525 to 2734843 Compare November 5, 2024 05:01
horgh
horgh requested changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@horgh horgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job finding these issues! I have a few suggestions.

src/maxminddb.c Outdated
@@ -732,6 +732,8 @@ static int populate_languages_metadata(MMDB_s *mmdb,
MMDB_entry_data_list_s *member;
status = MMDB_get_entry_data_list(&array_start, &member);
if (MMDB_SUCCESS != status) {
if (MMDB_OUT_OF_MEMORY_ERROR != status)
MMDB_free_entry_data_list(member);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a good find. However I think it would make sense to clean up inside MMDB_get_entry_data_list() if status is not MMDB_SUCCESS. Then its callers would not need to worry about it and we wouldn't need to do this here.

Copy link
Contributor Author

@pkillarjun pkillarjun Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

diff --git a/src/maxminddb.c b/src/maxminddb.c
index c7e7542..5c99134 100644
--- a/src/maxminddb.c
+++ b/src/maxminddb.c
@@ -1651,6 +1651,10 @@ int MMDB_get_entry_data_list(MMDB_entry_s *start,
 
     int const status =
         get_entry_data_list(start->mmdb, start->offset, list, pool, 0);
+    if (MMDB_SUCCESS != status) {
+        data_pool_destroy(pool);
+        return status;
+    }
 
     *entry_data_list = data_pool_to_list(pool);
     if (!*entry_data_list) {

This would work;

+ return status;

We have to return here. If we don't return, there will be a use-after-free bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side effect patch of this approach 69af4e0.

Copy link
Contributor Author

@pkillarjun pkillarjun Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit skeptical of this patch now because there could be more double-free or use-after-free like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's true. I think it is likely fine though. The function could already behave this way - the first two early returns would already cause that issue. This seems more like how I would expect the function to behave.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's true. I think it is likely fine though. The function could already behave this way - the first two early returns would already cause that issue. This seems more like how I would expect the function to behave.

Agree to disagree.

src/maxminddb.c Show resolved Hide resolved
src/maxminddb.c Outdated Show resolved Hide resolved
horgh
horgh approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@horgh horgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@horgh horgh merged commit 70abedb into maxmind:main Nov 8, 2024
15 checks passed
horgh added a commit that referenced this pull request Nov 8, 2024
@horgh
Add changelog entries for #356 and #357
80aebfa
oschwald added a commit that referenced this pull request Nov 11, 2024
@oschwald
Merge pull request #358 from maxmind/horgh/changelog
cab57f3 
Add changelog entries for #356 and #357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@horgh horgh horgh approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pkillarjun @horgh