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.

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

tests: Avoid accumulating allocated memory in a global state if LogPrintf is called when fuzzing #17894

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 7, 2020

Avoid accumulating allocated memory in a global state if LogPrintf is called when fuzzing.

The accumulation takes place via the m_msgs_before_open buffering.

Resolved by enabling logging and writing log messages to standard output.

This has the added benefit of making the fuzzing operator aware of any log printing caused by fuzzing which is likely to be an anomaly in itself (in the general case).

The only fuzzing harness in master that I've seen calling LogPrintf is messageheader_deserialize via the call to CMessageHeader::IsValid() which somewhat surprisingly does a LogPrintf in the case of nMessageSize > MAX_SIZE :)

Before:

$ src/test/fuzz/messageheader_deserialize corpus/
…
INFO: libFuzzer disabled leak detection after every mutation.
      Most likely the target function accumulates allocated
      memory in a global state w/o actually leaking it.
      You may try running this binary with -trace_malloc=[12]
      to get a trace of mallocs and frees.
      If LeakSanitizer is enabled in this process it will still
      run on the process shutdown.
…

After:

$ src/test/fuzz/messageheader_deserialize corpus/
…
2020-01-07T23:20:34Z CMessageHeader::IsValid(): (@, 4278190080 bytes) nMessageSize > MAX_SIZE
…

How to test this PR

$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/messageheader_deserialize
…

@fanquake fanquake added the Tests label Jan 7, 2020
@Crypt-iQ
Copy link
Contributor

Crypt-iQ commented Jan 8, 2020

Concept ACK.

I'm not too familiar with libFuzzer so I can't speak to that, but won't this buildup only occur if AFL is in persistent mode since then there's a loop rather than a new binary being executed?

@fanquake
Copy link
Member

fanquake commented Jan 8, 2020

Travis failure:

Traceback (most recent call last):
  File "test/fuzz/test_runner.py", line 175, in <module>
    main()
  File "test/fuzz/test_runner.py", line 115, in main
    universal_newlines=True,
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/addr_info_deserialize', '-help=1']' 
died with <Signals.SIGABRT: 6>.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 10, 2020

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

Conflicts

No conflicts as of last run.

@practicalswift
Copy link
Contributor Author

@Crypt-iQ Yes, the build-up will only occur if in-process fuzzing is used: libFuzzer is always in-process and AFL only when in persistent mode.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Concept ACK. Approach NACK.

src/test/fuzz/deserialize.cpp Show resolved Hide resolved
@practicalswift practicalswift force-pushed the fuzzers-deserialize-enable-logging branch from 125de8a to 91013dd Compare January 13, 2020 21:13
@practicalswift practicalswift force-pushed the fuzzers-deserialize-enable-logging branch from 91013dd to d7f4583 Compare January 14, 2020 12:11
@practicalswift
Copy link
Contributor Author

@MarcoFalke Approach ACK now with the updated version? :)

@practicalswift
Copy link
Contributor Author

@MarcoFalke How can we proceed with this one? Would be really nice to not have deal with this memory leak when fuzzing the deserialisation code :)

@practicalswift
Copy link
Contributor Author

Closing due to lack of interest :)

@practicalswift practicalswift deleted the fuzzers-deserialize-enable-logging branch April 10, 2021 19:39
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants