fuzz: Exit and log stderr for parse_test

dergoegge commented at 10:55 AM on January 24, 2024: member

We should log all errors that occur when attempting to print the harness list in the fuzz test runner.

DrahtBot commented at 10:55 AM on January 24, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jan 24, 2024

dergoegge commented at 10:56 AM on January 24, 2024: member

This is currently a problem in the qa-assets CI, listing the harnesses fails but there is no output: https://cirrus-ci.com/task/5433841842651136?logs=ci#L9235

...
+ test/fuzz/test_runner.py -j6 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/ --empty_min_time=60
No fuzz targets found

in test/fuzz/test_runner.py:375 in 6fe7d39a56 outdated

 372 |          env={
 373 |              'PRINT_ALL_FUZZ_TARGETS_AND_ABORT': ''
 374 |          },
 375 |          stdout=subprocess.PIPE,
 376 | -        stderr=subprocess.DEVNULL,
 377 | +        stderr=subprocess.STDOUT,

maflcko commented at 11:09 AM on January 24, 2024:

Why redirect it to stdout, then read stdout, and then print it to stderr? Wouldn't it be easier to just remove this line and thus inherit stderr?

dergoegge commented at 11:28 AM on January 24, 2024:

Inheriting is better, done!

in test/fuzz/test_runner.py:382 in 6fe7d39a56 outdated

 381 | +
 382 | +    if res.returncode != 0:
 383 | +        logging.error(f"Binary returned an error (code: {res.returncode}) while trying to list harnesses:\n{res.stdout}")
 384 | +        sys.exit(1)
 385 | +
 386 | +    test_list_all = res.stdout.splitlines()

maflcko commented at 11:11 AM on January 24, 2024:

This will now include stderr, because it was redirected to stdout, no? Seems easier to keep them separate?

maflcko changes_requested

dergoegge force-pushed on Jan 24, 2024

dergoegge renamed this:
~~fuzz: Exit and log stdout for parse_test_list errors~~
fuzz: Exit and log stderr for parse_test_list errors
on Jan 24, 2024

fuzz: Exit and log stderr for parse_test_list errors 9d09c873a5

in test/fuzz/test_runner.py:380 in 66b23000a1 outdated

 378 | -    ).stdout.splitlines()
 379 | +    )
 380 | +
 381 | +    if res.returncode != 0:
 382 | +        logging.error(f"Binary returned an error (code: {res.returncode}) while trying to list harnesses")
 383 | +        sys.exit(1)

maflcko commented at 11:39 AM on January 24, 2024:

This can also be removed, by simply using check=True,, to get the same details, and even a traceback, no?

dergoegge commented at 11:43 AM on January 24, 2024:

great, done!

dergoegge force-pushed on Jan 24, 2024

maflcko commented at 11:45 AM on January 24, 2024: member

lgtm ACK 9d09c873a50d344e2a9cb35fe246a52688b9fa34

simple +1-1 change

in test/fuzz/test_runner.py:376 in 9d09c873a5

 371 | @@ -372,8 +372,8 @@ def parse_test_list(*, fuzz_bin):
 372 |              'PRINT_ALL_FUZZ_TARGETS_AND_ABORT': ''
 373 |          },
 374 |          stdout=subprocess.PIPE,
 375 | -        stderr=subprocess.DEVNULL,
 376 |          text=True,
 377 | +        check=True,

maflcko commented at 11:47 AM on January 24, 2024:

For reference, this check is fine to do since fa22966f330 changed the abort into a successful exit.

fanquake merged this on Jan 24, 2024

fanquake closed this on Jan 24, 2024

fanquake commented at 9:49 AM on January 25, 2024: member

Looks like this is working as intended https://cirrus-ci.com/task/5482198250291200?logs=ci#L9646:

+ test/fuzz/test_runner.py -j6 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/ --empty_min_time=60
==29805==WARNING: MemorySanitizer: use-of-uninitialized-value
    [#0](/bitcoin-bitcoin/0/) 0x55ef0724925d  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x93625d)
    [#1](/bitcoin-bitcoin/1/) 0x55ef09320bb9  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x2a0dbb9)
    [#2](/bitcoin-bitcoin/2/) 0x55ef0717e664  (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x86b664)
    [#3](/bitcoin-bitcoin/3/) 0x7fd0ab3e9eba  (/lib/x86_64-linux-gnu/libc.so.6+0x29eba) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
Traceback (most recent call last):
  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 382, in <module>
    main()
  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 106, in main
    test_list_all = parse_test_list(fuzz_bin=os.path.join(config["environment"]["BUILDDIR"], 'src', 'test', 'fuzz', 'fuzz'))
  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 369, in parse_test_list
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz' returned non-zero exit status 1.

fanquake commented at 9:49 AM on January 25, 2024: member

We should also fix the symbolizing in that env.

bitcoin locked this on Jan 24, 2025

fuzz: Exit and log stderr for parse_test_list errors #29304

Code Coverage

Reviews