Skip to content

Commit

Permalink
Fix cord handling in DynamicMessage and oneofs. (#18374)
Browse files Browse the repository at this point in the history
* Fix cord handling in DynamicMessage and oneofs.

This fixes a memory corruption vulnerability for anyone using cord with dynamically built descriptor pools.

* Move -Werror to our test/dev bazelrc files. (#17938)

    * Move -Werror to our test/dev bazelrc files.

    Putting it into BUILD files unintentionally forces it on all our downstream users.  Instead, we just want to enable this during testing and let them choose for themselves in their builds.

    Note, that this expands the scope of -Werror to our entire repo for CI, so a bunch of fixes and opt-outs had to be applied to get this change passing.

    Closed #14714

    PiperOrigin-RevId: 666903224

    * Fix extra warnings on 28.x

    * Fix zlib issues on macos

    * Second try at zlib/macos issues

    * Only disable deprecated-non-prototype on macos-14

* Silence expected ubsan failures from absl::Cord

* Fix test dependency

* Fix python/upb warnings

* Fix last upb warning

---------

Co-authored-by: Mike Kruskal <[email protected]>
  • Loading branch information
zhangskz and mkruskal-google authored Sep 18, 2024
1 parent b96676e commit 6fa3f2d
Show file tree
Hide file tree
Showing 70 changed files with 822 additions and 583 deletions.
5 changes: 5 additions & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
build --cxxopt=-std=c++17 --host_cxxopt=-std=c++17

build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations"


build:dbg --compilation_mode=dbg

build:opt --compilation_mode=opt
Expand All @@ -22,6 +25,8 @@ build:ubsan --action_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1
# Workaround for the fact that Bazel links with $CC, not $CXX
# https://github.com/bazelbuild/bazel/issues/11122#issuecomment-613746748
build:ubsan --copt=-fno-sanitize=function --copt=-fno-sanitize=vptr
# Abseil passes nullptr to memcmp with 0 size
build:ubsan --copt=-fno-sanitize=nonnull-attribute

# TODO: migrate all dependencies from WORKSPACE to MODULE.bazel
# https://github.com/protocolbuffers/protobuf/issues/14313
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/test_objectivec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ jobs:
- OS: macos-14
PLATFORM: "visionos"
XCODE: "15.2"
EXTRA_FLAGS: --copt="-Wno-deprecated-non-prototype"
name: CocoaPods ${{ matrix.PLATFORM }} ${{ matrix.CONFIGURATION }}
runs-on: ${{ matrix.OS }}
steps:
Expand All @@ -91,7 +92,7 @@ jobs:
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
bazel-cache: cocoapods/${{ matrix.XCODE }}
bash: |
./regenerate_stale_files.sh $BAZEL_FLAGS --xcode_version="${{ matrix.XCODE }}"
./regenerate_stale_files.sh $BAZEL_FLAGS ${{ matrix.EXTRA_FLAGS }} --xcode_version="${{ matrix.XCODE }}"
pod lib lint --verbose \
--configuration=${{ matrix.CONFIGURATION }} \
--platforms=${{ matrix.PLATFORM }} \
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/test_rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ jobs:
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
bazel-cache: rust_linux
bazel: >-
test //rust:protobuf_upb_test //rust:protobuf_cpp_test
test --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 --copt="-Wno-return-type-c-linkage"
//rust:protobuf_upb_test //rust:protobuf_cpp_test
//rust/test/rust_proto_library_unit_test:rust_upb_aspect_test
//src/google/protobuf/compiler/rust/...
2 changes: 1 addition & 1 deletion .github/workflows/test_upb.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ jobs:
image: "us-docker.pkg.dev/protobuf-build/containers/test/linux/gcc:12.2-6.3.0-63dd26c0c7a808d92673a3e52e848189d4ab0f17"
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
bazel-cache: "upb-bazel-gcc"
bazel: test --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 -c opt //bazel/... //benchmarks/... //lua/... //protos/... //protos_generator/... //python/... //upb/... //upb_generator/...
bazel: test --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 -c opt --copt="-Wno-error=maybe-uninitialized" --copt="-Wno-error=attributes" //bazel/... //benchmarks/... //lua/... //protos/... //protos_generator/... //python/... //upb/... //upb_generator/...

windows:
strategy:
Expand Down
1 change: 0 additions & 1 deletion build_defs/cpp_opts.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ COPTS = select({
"-Woverloaded-virtual",
"-Wno-sign-compare",
"-Wno-nonnull",
"-Werror",
],
})

Expand Down
1 change: 1 addition & 0 deletions ci/Linux.bazelrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import common.bazelrc

build --cxxopt=-std=c++14 --host_cxxopt=-std=c++14
build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations"
2 changes: 2 additions & 0 deletions ci/common.bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ build:ubsan --action_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1
# Workaround for the fact that Bazel links with $CC, not $CXX
# https://github.com/bazelbuild/bazel/issues/11122#issuecomment-613746748
build:ubsan --copt=-fno-sanitize=function --copt=-fno-sanitize=vptr
# Abseil passes nullptr to memcmp with 0 size
build:ubsan --copt=-fno-sanitize=nonnull-attribute

# Workaround Bazel 7 remote cache issues.
# See https://github.com/bazelbuild/bazel/issues/20161
Expand Down
1 change: 1 addition & 0 deletions ci/macOS.bazelrc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import common.bazelrc

build --cxxopt=-std=c++14 --host_cxxopt=-std=c++14
build --copt="-Werror" --copt="-Wno-sign-compare" --copt="-Wno-sign-conversion" --copt="-Wno-error=sign-conversion" --copt="-Wno-deprecated-declarations"
common --repo_env=BAZEL_NO_APPLE_CPP_TOOLCHAIN=1
7 changes: 3 additions & 4 deletions conformance/binary_json_conformance_suite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3449,12 +3449,11 @@ BinaryAndJsonConformanceSuiteImpl<MessageType>::GetFieldForOneofType(
template <typename MessageType>
std::string BinaryAndJsonConformanceSuiteImpl<MessageType>::SyntaxIdentifier()
const {
if constexpr (std::is_same<MessageType, TestAllTypesProto2>::value) {
if (std::is_same<MessageType, TestAllTypesProto2>::value) {
return "Proto2";
} else if constexpr (std::is_same<MessageType, TestAllTypesProto3>::value) {
} else if (std::is_same<MessageType, TestAllTypesProto3>::value) {
return "Proto3";
} else if constexpr (std::is_same<MessageType,
TestAllTypesProto2Editions>::value) {
} else if (std::is_same<MessageType, TestAllTypesProto2Editions>::value) {
return "Editions_Proto2";
} else {
return "Editions_Proto3";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import com.google.protobuf.CodedOutputStream.OutOfSpaceException;
import protobuf_unittest.UnittestProto.SparseEnumMessage;
import protobuf_unittest.UnittestProto.TestAllTypes;
import protobuf_unittest.UnittestProto.TestPackedTypes;
import protobuf_unittest.UnittestProto.TestSparseEnum;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
Expand Down Expand Up @@ -402,47 +401,6 @@ public void computeTagSize() {
assertThat(CodedOutputStream.computeTagSize((1 << 30) + 1)).isEqualTo(1);
}

/** Tests writing a whole message with every field type. */
@Test
public void testWriteWholeMessage() throws Exception {
final byte[] expectedBytes = TestUtil.getGoldenMessage().toByteArray();
TestAllTypes message = TestUtil.getAllSet();

for (OutputType outputType : OutputType.values()) {
Coder coder = outputType.newCoder(message.getSerializedSize());
message.writeTo(coder.stream());
coder.stream().flush();
byte[] rawBytes = coder.toByteArray();
assertEqualBytes(outputType, expectedBytes, rawBytes);
}

// Try different block sizes.
for (int blockSize = 1; blockSize < 256; blockSize *= 2) {
Coder coder = OutputType.STREAM.newCoder(blockSize);
message.writeTo(coder.stream());
coder.stream().flush();
assertEqualBytes(OutputType.STREAM, expectedBytes, coder.toByteArray());
}
}

/**
* Tests writing a whole message with every packed field type. Ensures the wire format of packed
* fields is compatible with C++.
*/
@Test
public void testWriteWholePackedFieldsMessage() throws Exception {
byte[] expectedBytes = TestUtil.getGoldenPackedFieldsMessage().toByteArray();
TestPackedTypes message = TestUtil.getPackedSet();

for (OutputType outputType : OutputType.values()) {
Coder coder = outputType.newCoder(message.getSerializedSize());
message.writeTo(coder.stream());
coder.stream().flush();
byte[] rawBytes = coder.toByteArray();
assertEqualBytes(outputType, expectedBytes, rawBytes);
}
}

/**
* Test writing a message containing a negative enum value. This used to fail because the size was
* not properly computed as a sign-extended varint.
Expand Down
Loading

0 comments on commit 6fa3f2d

Please sign in to comment.