fuzz-introspector icon indicating copy to clipboard operation
fuzz-introspector copied to clipboard

Incorrect hyperlink in the calltree

Open wenta0g opened this issue 1 year ago • 4 comments

For example, see the call site link of the function igraph_i_parse_real in the igraph report, https://storage.googleapis.com/oss-fuzz-introspector/igraph/inspector-report/20230226/calltree_view_4.html?scrollToNode=00221

The call site of the aforementioned function in the original igraph library is this: https://github.com/igraph/igraph/blob/6de21b242e764c2a0a6cb7a6ae04aa70262dbc25/src/io/dl-parser.y#L229. The line numbers of functions and call sites are all taken from this file ending with ".y".

However, link in the call tree links to a C file in the coverage report which is converted from the .y file. This creates a discrepancy between all line numbers recorded from the original file and the converted C file. See the coverage report highlighting the incorrect line (this is the link to call cite of igraph_i_parse_real in the call tree): https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/build/src/io/parsers/dl-parser.c.html#L229.

Could this be a potential bug? Thank you.

wenta0g avatar Feb 28 '23 06:02 wenta0g

Interesting, thanks for reporting!

Am trying to understand what the true callsite should be, is it https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/build/src/io/parsers/dl-parser.c.html#L1845 ?

DavidKorczynski avatar Feb 28 '23 10:02 DavidKorczynski

dl-parser.c is generated from dl-parser.y by GNU Bison. The .y file is written with Bison syntax, but contains mostly C code.

Bison includes #line preprocessor directives into the output by default. This allows most tools to produce diagnostics that refer back to the .y file. igraph actually has this turned off by default (for privacy reasons, think file paths hardcoded in the source release), but it has a switch for its CMake-based build system to turn it on, -DFLEX_KEEP_LINE_NUMBERS=ON, which I used for OSS-fuzz: https://github.com/igraph/igraph/blob/master/fuzzing/build.sh#L23

This way, AddressSanitizer output, as well as compiler warnings, will actually refer to code written by humans (in the .y source file), rather than the generated .c files, whose layout depends on the Bison version.

The most useful location to refer to is in the .y file, line 229:

https://github.com/igraph/igraph/blob/6de21b242e764c2a0a6cb7a6ae04aa70262dbc25/src/io/dl-parser.y#L229

What you link to is also technically correct:

https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/build/src/io/parsers/dl-parser.c.html#L1845

But it is much less useful when working on igraph, as now I need to refer to the C sources generated specifically on the OSS-fuzz infrastructure, and relate them back the .y file on my system.

Note that it is this line in the generated C file that allows the compiler to refer back to the .y file automatically:

https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/build/src/io/parsers/dl-parser.c.html#L1842

szhorvat avatar Feb 28 '23 10:02 szhorvat

Just going by the name of this ticket, this is another odd behavior, less interesting than how to handle intermediate generated source files which feels like an enhancement issue where certainly if attribution to the authored source is possible it should be supported.

Looking at https://storage.googleapis.com/oss-fuzz-introspector/htslib/inspector-report/20230124/fuzz_report.html and viewing first fuzz-blocker cram_close call site: 03338

The call site link is to the line above (hts_tpool_process_destroy) what is the actual call site of cram_close, cram_write_eof_block.

eantelman avatar Mar 03 '23 17:03 eantelman

Thanks for the description @szhorvat

I think there are two issues at play here.

The callsite here https://storage.googleapis.com/oss-fuzz-introspector/igraph/inspector-report/20230226/calltree_view_4.html?scrollToNode=00221 is set to "red" because the line it refers back to "https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/build/src/io/parsers/dl-parser.c.html#L229" which is not covered. This, however, is wrong information because the line number is taken from the *.y file (229) whereas it references the generated *.c file where the corresponding callsite in the coverage report is 1845 (https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/build/src/io/parsers/dl-parser.c.html#L1845). And in the coverage report, line 229 is irrelevant to the callsite.

There are two problems:

  1. The coverage indication is wrong, because it mingles the line numbers as described above.
  2. The link should be to *.y file rather than *.c file, because developers deal only with the *.y file and not really deal with the *.c file (which furthermore depends on the Bison version used).

Solution to (1) above should probably be fairly intuitive to come up with -- I'll look at this.

Solution to (2) is a bit different. Currently the callsite pages provide links to files in the code coverage reports, and the *.y file is not part of the code coverage report, if it was it should be here https://storage.googleapis.com/oss-fuzz-coverage/igraph/reports-by-target/20230226/read_dl_fuzzer/linux/src/igraph/src/io/report.html. As such, we need to either (2.1) include a link back to the source repository where the *.y file exists or (2.2) include the *.y file in the code coverage report, and get the coverage reporting utilities to use these. I think (2.1) is easier solvable because it's in our control, whereas (2.2) may not be fully in our control. I think coming up with (2.1) should be possible.

DavidKorczynski avatar Mar 04 '23 17:03 DavidKorczynski