integer overflow in protobuf while processing components/metrics .proto files |
|||||||||
Issue descriptionI was doing a local libfuzzer ubsan build for something ( issue 696729 ) and got this: [10612/21334] ACTION //components/metrics/proto:proto_gen(//build/toolchain/linux:clang_x64) ../../third_party/protobuf/src/google/protobuf/stubs/strutil.cc:984:17: runtime error: signed integer overflow: 42 * 100000000 cannot be represented in type 'int' It's just a warning message and doesn't block me, but it's probably not good that this is happening.
,
Apr 19 2017
All other protobuf files are processed fine, so it's metrics's .proto files triggering something in protobuf that other .proto files don't do. So someone with metrics domain knowledge is probably better equipped to take this.
,
Apr 19 2017
Ah - I think I missed that it's a runtime error (ubsan) and not a compile error. Fair enough. Can you provide gn args and build command line to repro?
,
Apr 19 2017
$ cat out/libfuzzer/args.gn is_debug = false use_libfuzzer = true is_ubsan_security = true enable_nacl = false proprietary_codecs = true ffmpeg_branding = "ChromeOS" use_goma=true $ export FUZZER_NAME=renderer_tree_fuzzer $ ninja -C out/libfuzzer $FUZZER_NAME -j500
,
Apr 21 2017
I'll take a look.
,
Apr 21 2017
hmm, I get some errors trying to use the above gn args: //third_party/webrtc/test/fuzzers:neteq_rtp_fuzzer(//build/toolchain/linux:clang_x64) needs //third_party/webrtc/base:rtc_base_tests_utils(//build/toolchain/linux:clang_x64) //third_party/webrtc/test/fuzzers:neteq_rtp_fuzzer(//build/toolchain/linux:clang_x64) needs //third_party/webrtc/modules/audio_coding:neteq_unittest_tools(//build/toolchain/linux:clang_x64) //third_party/webrtc/test/fuzzers:ulpfec_generator_fuzzer(//build/toolchain/linux:clang_x64) needs //third_party/webrtc/modules/rtp_rtcp:rtp_rtcp_unittests(//build/toolchain/linux:clang_x64) Do I need something special to get those deps?
,
Apr 21 2017
I'm unable to find the error message in that snippet :-/ https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md are the instructions, I think they mean things are supposed to just work. Are you synced up? I have src-internal checked out, not sure if that's needed.
,
Apr 21 2017
I am synced up and do have src internal. To clarify, the error is when specifying those args via the "gn args" command line: Generating files... ERROR Unresolved dependencies. //third_party/webrtc/test/fuzzers:neteq_rtp_fuzzer(//build/toolchain/linux:clang_x64) needs //third_party/webrtc/base:rtc_base_tests_utils(//build/toolchain/linux:clang_x64) //third_party/webrtc/test/fuzzers:neteq_rtp_fuzzer(//build/toolchain/linux:clang_x64) needs //third_party/webrtc/modules/audio_coding:neteq_unittest_tools(//build/toolchain/linux:clang_x64) //third_party/webrtc/test/fuzzers:ulpfec_generator_fuzzer(//build/toolchain/linux:clang_x64) needs //third_party/webrtc/modules/rtp_rtcp:rtp_rtcp_unittests(//build/toolchain/linux:clang_x64) Note: I also got those on my Mac - so I tried on Linux just in case. (The Mac error is the same except it has "mac:" instead of "linux:" in the messages.) I can look at the more detailed libfuzzer instructions in case there's something I'm missing when I have a chance, I guess.
,
Apr 21 2017
I'm getting this error too. This is a recent regression, it worked fine 2 days ago when I filed the bug.
,
Apr 21 2017
mbonadei touched third_party/webrtc/test/fuzzers/BUILD.gn 2 days ago, it probably broke fuzzers in Chromium (see previous 2 comments)
,
Apr 22 2017
Hi, in WebRTC we changed webrtc/test/fuzzers/BUILD.gn in these two CLs this week: https://codereview.webrtc.org/2815103005 and https://codereview.webrtc.org/2717083002. Looking at the error that you pasted in comment #8 it seems that 'webrtc/test/fuzzers:neteq_rtp_fuzzer' needs 'webrtc/base:rtc_base_tests_utils' and it has been set as a dependency in https://codereview.webrtc.org/2717083002 (https://cs.chromium.org/chromium/src/third_party/webrtc/test/fuzzers/BUILD.gn?q=third_party/webrtc/test/fu+package:%5Echromium$&l=279). Can you please check that you have 'webrtc/base:rtc_base_tests_utils' in the deps list of 'webrtc/test/fuzzers:neteq_rtp_fuzzer'?
,
Apr 23 2017
We're doing a regular chromium build here. Can you try doing that yourself please?
,
Apr 24 2017
This is caused by the fact that our WebRTC test and tool targets have traditionally been hidden from Chromium (to avoid slowing down the Chromium build). Now that they're referenced by the libfuzzer tests, we need to move them out of the rtc_include_tests conditions in our GN files. Mirko will take care of that. I guess this may affect all the above mentioned deps. webrtc/base:rtc_base_tests_utils webrtc/modules/audio_coding:neteq_unittest_tools webrtc/modules/rtp_rtcp:rtp_rtcp_unittests The last one would preferably be refactored to something smaller so Chromium doesn't have to build the whole set of tests (which are not used). I filed a separate bug to update our libfuzzer bot in standalone WebRTC to cover this config: bug 714531
,
Apr 24 2017
This sounds a bit involved -- can we revert to green in the meantime?
,
Apr 24 2017
I am trying to land https://codereview.webrtc.org/2840523003/ as soon as possible to fix this, it is running on the chromium tryobots.
,
Apr 24 2017
Ok, the CL is now landed. I will follow up later today to be sure that everything is now working.
,
Apr 24 2017
Issue 714681 has been merged into this issue.
,
Apr 24 2017
We had to revert the CL in #16 so we're restoring a good state by reverting the two CLs in WebRTC: https://codereview.webrtc.org/2838683002/ (landed) https://codereview.webrtc.org/2842533002/ (lands soon) Please revert breaking WebRTC roll in Chromium DEPS if you want a quicker fix, or wait for a new roll to be made by emircan@ as soon both the above has landed.
,
Apr 24 2017
I confirm that the two reverts are landed [1]. I tried to generate a project with: is_debug = false use_libfuzzer = true is_ubsan_security = true enable_nacl = false proprietary_codecs = true ffmpeg_branding = "ChromeOS" And I am not hitting the dependency problem. [1] - https://chromium.googlesource.com/chromium/src.git/+/5cf00322124a19db68f811f0c2740243c48ac6cb
,
Apr 25 2017
All the libfuzzer bots are green since the WebRTC roll in https://chromium.googlesource.com/chromium/src/+/5cf00322124a19db68f811f0c2740243c48ac6cb We have updated the libfuzzer trybot we have in WebRTC to prevent the same type of problem happening again (setting rtc_include_tests=false).
,
Apr 25 2017
Sorry about the rushed closing of this bug. I guess the right thing is to bring it back to the state of comment #5 now.
,
Apr 25 2017
Okay, back to the original bug, the error appears to happen when processing perf_data.proto. I've narrowed it down to be caused by perfEventAttr message having at least 32 fields. Removing fields below that makes it go away.
Here's an attached minimal repro case of that proto that still fails:
syntax = "proto2";
package metrics;
message PerfEventAttr {
optional uint32 type = 1;
optional uint32 size = 2;
optional uint64 config = 3;
optional uint64 sample_period = 4;
optional uint64 sample_freq = 5;
optional uint64 sample_type = 6;
optional uint64 read_format = 7;
optional bool disabled = 8;
optional bool inherit = 9;
optional bool pinned = 10;
optional bool exclusive = 11;
optional bool exclude_user = 12;
optional bool exclude_kernel = 13;
optional bool exclude_hv = 14;
optional bool exclude_idle = 15;
optional bool mmap = 16;
optional bool comm = 17;
optional bool freq = 18;
optional bool inherit_stat = 19;
optional bool enable_on_exec = 20;
optional bool task = 21;
optional bool watermark = 22;
optional uint32 precise_ip = 23;
optional bool mmap_data = 24;
optional bool sample_id_all = 25;
optional bool exclude_host = 26;
optional bool exclude_guest = 27;
optional uint32 wakeup_events = 28;
optional uint32 wakeup_watermark = 29;
optional uint32 bp_type = 30;
optional uint64 bp_addr = 31;
optional uint64 config1 = 32;
}
,
Apr 25 2017
The specific bad value being encoded is 4278190080. It does appear to be in generated output file: https://cs.chromium.org/chromium/src/out/Debug/gen/components/metrics/proto/perf_data.pb.cc?rcl=abb796c8b2aab42c4aefa2f0a1a29d5aebd314f4&l=284
,
Apr 25 2017
I think solution is to change the digits var in FastUInt32ToBufferLeft() to a uint32, so its type matches |u|. I'll send a pull request to protobuf project.
,
Apr 25 2017
And of course this change is already the case in upstream protobuf library: https://github.com/google/protobuf/blob/f983302ca7051bd1364a6ea9218e21040e4a851b/src/google/protobuf/stubs/strutil.cc#L984 So we just need to roll it, I guess.
,
Apr 25 2017
Assigning to pkasting@ from OWNERS file of third_party/protobuf/ to roll to a newer version to pick up upstream fixes. Thanks!
,
Nov 29 2017
We picked this up in https://chromium.googlesource.com/chromium/src/+/ac47edd22c481fcfe119769d6b7abf365abea8fa . |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by asvitk...@chromium.org
, Apr 19 2017