stcorp / harp

Data harmonization toolset for scientific earth observation data
http://stcorp.github.io/harp/doc/html/index.html
BSD 3-Clause "New" or "Revised" License
55 stars 18 forks source link

harp_program_from_string_fuzzer: Direct-leak in harp_program_new #217

Closed schwehr closed 4 years ago

schwehr commented 4 years ago
    #1 0x55d41ae87aee in harp_program_new third_party/stcorp_harp/libharp/harp-program.c:47:31
    #2 0x55d41ae6da62 in harp_operation_parser_parse libharp/harp-operation-parser.y:1682:17
    #3 0x55d41ae776c5 in harp_program_from_string libharp/harp-operation-parser.y:1700:9
    #4 0x55d41ac712cc in LLVMFuzzerTestOneInput third_party/stcorp_harp/fuzz/harp_program_from_string_fuzzer.cc:19:3

The test case is just ?

testcase-5639552399572992.zip

svniemeijer commented 4 years ago

I can't reproduce this. Are you perhaps using a yacc version that does not support %destructor?

schwehr commented 4 years ago

It's being built on debian testing.

bison --version
bison (GNU Bison) 3.5.1

In the Makefile after running configure, I see:

YACC = bison -y

From the build:

/bin/sh ./ylwrap libharp/harp-operation-parser.y y.tab.c libharp/harp-operation-parser.c y.tab.h `echo libharp/harp-operation-parser.c | sed -e s/cc$/hh/ -e s/cpp$/hpp/ -e s/cxx$/hxx/ -e s/c++$/h++/ -e s/c$/h/` y.output libharp/harp-operation-parser.output -- bison -y -d 
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y:286.1-11: warning: POSIX Yacc does not support %destructor [-Wyacc]
  286 | %destructor { harp_sized_array_delete($$); } double_array int32_array string_array identifier_array dimension_array dimensionspec
      | ^~~~~~~~~~~
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y:287.1-11: warning: POSIX Yacc does not support %destructor [-Wyacc]
  287 | %destructor { harp_operation_delete($$); } operation
      | ^~~~~~~~~~~
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y:288.1-11: warning: POSIX Yacc does not support %destructor [-Wyacc]
  288 | %destructor { harp_program_delete($$); } program
      | ^~~~~~~~~~~
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y:289.1-11: warning: POSIX Yacc does not support %destructor [-Wyacc]
  289 | %destructor { free($$); } STRING_VALUE INTEGER_VALUE DOUBLE_VALUE NAME UNIT identifier
      | ^~~~~~~~~~~
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y:291.1-14: warning: POSIX Yacc does not support %error-verbose [-Wyacc]
  291 | %error-verbose
      | ^~~~~~~~~~~~~~
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y:291.1-14: warning: deprecated directive: ‘%error-verbose’, use ‘%define parse.error verbose’ [-Wdeprecated]
  291 | %error-verbose
      | ^~~~~~~~~~~~~~
      | %define parse.error verbose
/usr/local/google/home/schwehr/src/harp/libharp/harp-operation-parser.y: warning: fix-its can be applied.  Rerun with option '--update'. [-Wother]
libharp/harp-operation-parser.h is unchanged

The generated code:

harp-operation-parser-generated.zip

This fixes the leak locally for me. In harp_program_from_string():

git diff libharp/harp-operation-parser.y
diff --git a/libharp/harp-operation-parser.y b/libharp/harp-operation-parser.y
index 532bfb76..42476b6b 100644
--- a/libharp/harp-operation-parser.y
+++ b/libharp/harp-operation-parser.y
@@ -1699,6 +1699,10 @@ int harp_program_from_string(const char *str, harp_program **program)
     bufstate = (void *)harp_operation_parser__scan_string(str);
     if (harp_operation_parser_parse() != 0)
     {
+        if (parsed_program != NULL)
+        {
+            harp_program_delete(parsed_program);
+        }
         if (harp_errno == 0)
         {
             harp_set_error(HARP_ERROR_OPERATION_SYNTAX, NULL);
svniemeijer commented 4 years ago

I was using Apple's leaks program to test it, but apparently that doesn't pick it up. I am now able to reproduce it with valgrind on Linux. Fix will arrive shortly.

svniemeijer commented 4 years ago

Fixed in aa1c572de6ee562bac552a6c27d710123c151962

schwehr commented 4 years ago

Verified