New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 713179 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

integer overflow in protobuf while processing components/metrics .proto files

Project Member Reported by thakis@chromium.org, Apr 19 2017

Issue description

I 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.
 
Seems this is a bug in protobuf library? Can you route it to them?

(I think "processing components/metrics" file is a red herring - as this is just a build of protobuf library which is needed for the components/metrics build.)

Comment 2 by thakis@chromium.org, 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.
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?

Comment 4 by thakis@chromium.org, 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
Owner: asvitk...@chromium.org
Status: Assigned (was: Untriaged)
I'll take a look.
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?

Comment 7 by thakis@chromium.org, 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.
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.

Comment 9 by thakis@chromium.org, 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.
Cc: mbonadei@chromium.org kjellander@chromium.org
mbonadei touched third_party/webrtc/test/fuzzers/BUILD.gn 2 days ago, it probably broke fuzzers in Chromium (see previous 2 comments)
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'?
We're doing a regular chromium build here. Can you try doing that yourself please?
Owner: mbonadei@chromium.org
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 

This sounds a bit involved -- can we revert to green in the meantime?
I am trying to land https://codereview.webrtc.org/2840523003/ as soon as possible to fix this, it is running on the chromium tryobots.
Ok, the CL is now landed. I will follow up later today to be sure that everything is now working.
Issue 714681 has been merged into this issue.
Labels: -Pri-3 Pri-2
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.
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
Status: Fixed (was: Assigned)
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).
Owner: asvitk...@chromium.org
Status: Assigned (was: Fixed)
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.
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;
}

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
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.
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.
Owner: pkasting@chromium.org
Assigning to pkasting@ from OWNERS file of third_party/protobuf/ to roll to a newer version to pick up upstream fixes. Thanks!
Owner: thomasanderson@chromium.org
Status: Fixed (was: Assigned)
We picked this up in https://chromium.googlesource.com/chromium/src/+/ac47edd22c481fcfe119769d6b7abf365abea8fa .

Sign in to add a comment