Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876793002:
Added AudioProcessing fuzzer (Closed)
|
Created:
3 years, 7 months ago by aleloi Modified:
3 years, 6 months ago Reviewers:
hlundin-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded new AudioProcessing fuzzer
This is a high-level fuzzer that creates an AudioProcessing instance.
All possible combinations of publicly visible components are
fuzzed. Input and output sample rate, call order and use of the
float/fix interface is fuzzed. Sample rate may change between calls.
To fuzz floating point numbers, raw data is converted to floats, and
filtered for special values like 'inf', 'nan' and very large
values. Note that the default use case of APM is to only allow values
between +/- 2^15.
BUG=webrtc:7820
Review-Url: https://codereview.webrtc.org/2876793002
Cr-Commit-Position: refs/heads/master@{#18678}
Committed: https://chromium.googlesource.com/external/webrtc/+/8c51282f7fd7e712628aba1677a9733d903eb219
Patch Set 1 : Almost reviewable shape: added float processing, minor cleanup. #Patch Set 2 : cleanup #Patch Set 3 : comments #
Total comments: 24
Patch Set 4 : Bugfixes, response to comments. #Patch Set 5 : Minor change. #
Total comments: 4
Patch Set 6 : Fuzzed ProcessStream may now return error codes. #
Messages
Total messages: 36 (25 generated)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Description was changed from ========== Moar audio fuzzers! Work on this for a while, run locally, then submit! (It's not ready yet). BUG=Add one later. ========== to ========== Added new AudioProcessing fuzzer This is a high-level fuzzer that creates an AudioProcessing instance. All possible combinations of publicly visible components are fuzzed. Input and output sample rate, call order and use of the float/fix interface is fuzzed. Sample rate may change between calls. To fuzz floating point numbers, raw data is converted to floats, and filtered for special values like 'inf', 'nan' and very large values. Note that the default use case of APM is to only allow values between +/- 2^15. BUG=webrtc:7820 ==========
aleloi@webrtc.org changed reviewers: + henrika@webrtc.org
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Hi! PTAL on my CL which adds a fuzzer for the audio processing module.
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org - henrika@webrtc.org
Hi! PTAL on my CL which adds a fuzzer for the AudioProcessing module.
Nice! I have a few comments only. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... File webrtc/test/fuzzers/audio_processing_fuzzer.cc (right): https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:60: bool ParseSequence(size_t size, ParseSequence and FuzzAudioProcessingFixed are only used in this file. Enclose them in an unnamed namespace. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:64: const size_t data_size = sizeof(float) * size; data_size_bytes https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:64: const size_t data_size = sizeof(float) * size; Shouldn't it be sizeof(T)? https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:76: void FuzzAudioProcessingFixed(const uint8_t* data, Why is it called Fixed? https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:91: const auto input_rate = ByteToNativeRate(*input_rate_byte); {input, output}_rate_hz https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:94: const size_t samples_per_input_channel = This is the number of samples per millisecond. Is that what you wanted? I would have guessed the block size should be 10 ms. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:105: float_frame.begin())) { I would have expected &float_frame[0], but I assume that the iterator that begin() returns converts easily to a float*. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:106: break; Why not simply return? https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:110: break; return? https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:125: break; return? https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:145: FuzzAudioProcessingFixed(data, size, false, apm.get()); Why not let fixed/float also depend on the fuzzer data, instead of calling both fixed and float in the same round? https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... File webrtc/test/fuzzers/audio_processing_fuzzer_configs.cc (right): https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer_configs.cc:28: auto ii = ParseBool(data, remaining_size); Suggest ie for Intelligibility Enhancer.
Patchset #4 (id:120001) has been deleted
Thanks for the comments! You spotted that the fixed-mode didn't work. PTAL on next patch set! https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... File webrtc/test/fuzzers/audio_processing_fuzzer.cc (right): https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:60: bool ParseSequence(size_t size, On 2017/06/16 14:48:27, hlundin-webrtc wrote: > ParseSequence and FuzzAudioProcessingFixed are only used in this file. Enclose > them in an unnamed namespace. Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:64: const size_t data_size = sizeof(float) * size; On 2017/06/16 14:48:27, hlundin-webrtc wrote: > Shouldn't it be sizeof(T)? Thanks, missed that! https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:64: const size_t data_size = sizeof(float) * size; On 2017/06/16 14:48:27, hlundin-webrtc wrote: > data_size_bytes Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:76: void FuzzAudioProcessingFixed(const uint8_t* data, On 2017/06/16 14:48:26, hlundin-webrtc wrote: > Why is it called Fixed? In another version, I had FuzzFixed and FuzzFloating. I merged them to have less code duplication. I've removed 'Fixed' now. Thanks! https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:91: const auto input_rate = ByteToNativeRate(*input_rate_byte); On 2017/06/16 14:48:27, hlundin-webrtc wrote: > {input, output}_rate_hz Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:94: const size_t samples_per_input_channel = On 2017/06/16 14:48:27, hlundin-webrtc wrote: > This is the number of samples per millisecond. Is that what you wanted? I would > have guessed the block size should be 10 ms. Wow, that actually led to the fix-point interface returning kBadDataLengthError. Which I didn't check for. Thanks, now fixed! https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:105: float_frame.begin())) { On 2017/06/16 14:48:27, hlundin-webrtc wrote: > I would have expected &float_frame[0], but I assume that the iterator that > begin() returns converts easily to a float*. I started thinking about whether this is safe and why it works. In the C++ doc page [1] for 'array', it says that the type of 'begin()' is RandomAccessIterator [2]. Looking at that doc, I see no conversion operators for a T*. Looking at the implementation [3] of std::array with which GN compiles our code, the type returned by 'begin()' is 'value_type*', with 'value_type=T'. In conclusion, this code works with our old (C++11 with some features missing) version of the standard library, but will stop working when we update to a more recent version. [1] http://en.cppreference.com/w/cpp/container/array [2] http://en.cppreference.com/w/cpp/concept/RandomAccessIterator [3] file //build/linux/debian_wheezy_amd64-sysroot/usr/include/c++/4.6/array in the WebRTC checkout https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:106: break; On 2017/06/16 14:48:26, hlundin-webrtc wrote: > Why not simply return? Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:110: break; On 2017/06/16 14:48:27, hlundin-webrtc wrote: > return? Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:125: break; On 2017/06/16 14:48:27, hlundin-webrtc wrote: > return? Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:145: FuzzAudioProcessingFixed(data, size, false, apm.get()); On 2017/06/16 14:48:27, hlundin-webrtc wrote: > Why not let fixed/float also depend on the fuzzer data, instead of calling both > fixed and float in the same round? Done. https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... File webrtc/test/fuzzers/audio_processing_fuzzer_configs.cc (right): https://codereview.webrtc.org/2876793002/diff/100001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer_configs.cc:28: auto ii = ParseBool(data, remaining_size); On 2017/06/16 14:48:27, hlundin-webrtc wrote: > Suggest ie for Intelligibility Enhancer. Done.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
LGTM with one more comment/question/thought. https://codereview.webrtc.org/2876793002/diff/150001/webrtc/test/fuzzers/audi... File webrtc/test/fuzzers/audio_processing_fuzzer.cc (right): https://codereview.webrtc.org/2876793002/diff/150001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:113: RTC_DCHECK_EQ(apm_return_code, AudioProcessing::kNoError); Hmmm. Will the live fuzzer be run with dcheck_always_on? And, what do we want the fuzzer to check? I mean, in general it is rather expected that a function returns an error when inserting fuzzed data. The problem you tried to get around was that the fuzzer did nothing because of a setup problem. Can you instead explicitly check that kBadDataLengthError is not returned? https://codereview.webrtc.org/2876793002/diff/150001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:120: RTC_DCHECK_EQ(apm_return_code, AudioProcessing::kNoError); Same here.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2876793002/#ps170001 (title: "Fuzzed ProcessStream may now return error codes.")
https://codereview.webrtc.org/2876793002/diff/150001/webrtc/test/fuzzers/audi... File webrtc/test/fuzzers/audio_processing_fuzzer.cc (right): https://codereview.webrtc.org/2876793002/diff/150001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:113: RTC_DCHECK_EQ(apm_return_code, AudioProcessing::kNoError); On 2017/06/20 09:32:58, hlundin-webrtc wrote: > Hmmm. Will the live fuzzer be run with dcheck_always_on? And, what do we want > the fuzzer to check? I mean, in general it is rather expected that a function > returns an error when inserting fuzzed data. The problem you tried to get around > was that the fuzzer did nothing because of a setup problem. Can you instead > explicitly check that kBadDataLengthError is not returned? Done. https://codereview.webrtc.org/2876793002/diff/150001/webrtc/test/fuzzers/audi... webrtc/test/fuzzers/audio_processing_fuzzer.cc:120: RTC_DCHECK_EQ(apm_return_code, AudioProcessing::kNoError); On 2017/06/20 09:32:58, hlundin-webrtc wrote: > Same here. Done.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18383)
On 2017/06/20 09:51:18, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18383) FYI: https://chromium-review.googlesource.com/c/541279/
The CQ bit was checked by aleloi@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1497961478606730,
"parent_rev": "be457570288c7126a11382d23c23d52c4427bf82", "commit_rev":
"8c51282f7fd7e712628aba1677a9733d903eb219"}
Message was sent while issue was closed.
Description was changed from ========== Added new AudioProcessing fuzzer This is a high-level fuzzer that creates an AudioProcessing instance. All possible combinations of publicly visible components are fuzzed. Input and output sample rate, call order and use of the float/fix interface is fuzzed. Sample rate may change between calls. To fuzz floating point numbers, raw data is converted to floats, and filtered for special values like 'inf', 'nan' and very large values. Note that the default use case of APM is to only allow values between +/- 2^15. BUG=webrtc:7820 ========== to ========== Added new AudioProcessing fuzzer This is a high-level fuzzer that creates an AudioProcessing instance. All possible combinations of publicly visible components are fuzzed. Input and output sample rate, call order and use of the float/fix interface is fuzzed. Sample rate may change between calls. To fuzz floating point numbers, raw data is converted to floats, and filtered for special values like 'inf', 'nan' and very large values. Note that the default use case of APM is to only allow values between +/- 2^15. BUG=webrtc:7820 Review-Url: https://codereview.webrtc.org/2876793002 Cr-Commit-Position: refs/heads/master@{#18678} Committed: https://chromium.googlesource.com/external/webrtc/+/8c51282f7fd7e712628aba167... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:170001) as https://chromium.googlesource.com/external/webrtc/+/8c51282f7fd7e712628aba167... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
