Fix libyaml_emitter_fuzzer: inverted copy_event return value check#15096
Conversation
copy_event() returns 1 on success and 0 on failure (standard libyaml convention). However, line 250 checks `if (copy_event(...))` which enters the error path on SUCCESS and continues on FAILURE. This means every valid YAML input triggers the error path on the very first event, making the entire emitter path dead code: - yaml_emitter_emit() is never called - The second parser loop (re-parse emitted output) is never reached - The event comparison logic is never executed Roughly 50% of the fuzzer's logic has never been tested. Additionally: - event_number++ was inside the copy_event call (post-increment), so on failure the index advances and cleanup deletes an uninitialized event (UB). Moved increment after success check. - Added NULL check for out.buf before passing to yaml_parser_set_input_string(), which asserts on NULL input. This was another latent bug masked by the same logic inversion. Coverage comparison (60s, AddressSanitizer, libFuzzer fork mode): | Metric | Original | Fixed | Change | |----------|----------|--------|------------| | Edges | 166 | 2479 | **+1393%** | | Features | 437 | 10285 | **+2253%** | | Corpus | 135 | 2258 | +1573% | The ~15x edge coverage increase confirms the emitter, write handler, and re-parser code paths were entirely dead in the original.
|
OwenSanzas is a new contributor to projects/libyaml. The PR must be approved by known contributors before it can be merged. The past contributors are: arthurscchan, hunsche, perlpunk, ingydotnet, alex, rjotwani, cvediver, Dor1s, ssbr (unverified), sigmavirus24 (unverified), inferno-chromium (unverified), mikea (unverified) |
DavidKorczynski
left a comment
There was a problem hiding this comment.
nice! Do you have any estimate for how much overall coverage gain from this? One part is the coverage of the harness itself, but the project overall has very high coverage already: https://storage.googleapis.com/oss-fuzz-coverage/libyaml/reports/20260304/linux/src/report.html
|
Thanks! I ran
The original emitter fuzzer hits 0% of |
DavidKorczynski
left a comment
There was a problem hiding this comment.
thanks for the clarifications
|
Thanks for the approval! |
…ter, #15096) Per user direction we cannot use the OpenSSL provider example because that paper is still under review. Switched to a publicly-visible, already-merged OSS-Fuzz PR: google/oss-fuzz#15096 Fix libyaml_emitter_fuzzer: inverted copy_event return value check The bug is a single inverted boolean: copy_event() follows the libyaml convention 'success returns 1', but the harness treats 1 as failure and short-circuits to the error-cleanup goto on every successful copy. Result: ~50% of the harness (the entire emitter pipeline plus the second re-parser loop) is dead code. Fix is one '!' character. After merging, edge coverage went from 166 to 2479 (+1393%, ~15x). Maps cleanly to P2.4 (Return value handling). Cited PR is public and linkable.
Summary
The
libyaml_emitter_fuzzerhas an inverted return value check that makes the entire emitter path dead code.Bug 1: Inverted logic in copy_event check
copy_event()wrapsyaml_*_event_initialize()functions, which return 1 on success, 0 on failure (standard libyaml convention). However, the check on line 250:enters the error path when
copy_eventsucceeds (returns 1). For every valid YAML input, the fuzzer short-circuits on the very first event. The following code is never reached:yaml_emitter_emit()— the entire emitter pathevents_equal()comparison logicRoughly 50% of the fuzzer's logic is dead code.
Fix:
if (copy_event(...))→if (!copy_event(...))Bug 2: Pre-increment causes UB on failure path
event_number++was inside thecopy_event()call as a post-increment. Whencopy_eventfails (returns 0 — rare, OOM only),event_numberhas already been incremented, butevents[old_number]was not initialized. The cleanup loop then callsyaml_event_delete()on an uninitialized event — undefined behavior.Fix: Move
event_number++to after the success check.Bug 3: NULL buffer passed to yaml_parser_set_input_string
When the emitter produces no output (e.g., empty YAML stream),
out.bufremains NULL. The second parser loop passes this NULL toyaml_parser_set_input_string(), which hasassert(input). This was masked by Bug 1 since the second parser loop was never reached.Fix: Add
if (!out.buf || out.size == 0) goto error;before the second parser loop.Coverage comparison (60 seconds, AddressSanitizer, libFuzzer fork mode)
The ~15x edge coverage increase confirms that the emitter, write handler, and re-parser code paths were entirely dead in the original fuzzer.