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

Fix oss-fuzz build compilation errors #18919

Merged
merged 4 commits into from
Jan 6, 2022

Conversation

disconnect3d
Copy link
Contributor

This PR fixes two compilation error that are happening in the oss-fuzz build of Envoy. Note that the latest oss-fuzz build errors out with only the error related to AsyncStream but AFAIR when it is fixed, a different compilation error has to be fixed which I fixed in the first commit that changes the socket.h file.

@repokitteh-read-only
Copy link

Hi @disconnect3d, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18919 was opened by disconnect3d.

see: more, trace.

Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

/wait

@@ -23,7 +23,6 @@ namespace Network {
// avoid #ifdef proliferation.
struct SocketOptionName {
SocketOptionName() = default;
SocketOptionName(const SocketOptionName&) = default;
SocketOptionName& operator=(const SocketOptionName&) = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also remove declaration of the default assignment operator. Same below.

@yanavlasov
Copy link
Contributor

Please also fix DCO. You can click on the "Details" link next to it for instructions.

@yanavlasov yanavlasov self-assigned this Nov 10, 2021
@adisuissa
Copy link
Contributor

@KBaichoo we initially added the default assignment operators to get around a similar issue.
I guess that if the copy c'tor isn't used then both can be removed.

Fixes an oss-fuzz Envoy compilation error:
```
Execution platform: @local_config_platform//:host
In file included from source/common/network/socket_option_impl.cc:1:
In file included from ./source/common/network/socket_option_impl.h:6:
In file included from ./envoy/network/listen_socket.h:14:
./envoy/network/socket.h:26:3: error: definition of implicit copy assignment operator for 'SocketOptionName' is deprecated because it has a user-declared copy constructor [-Werror,-Wdeprecated-copy]
  SocketOptionName(const SocketOptionName&) = default;
  ^
source/common/network/socket_option_impl.cc:52:14: note: in implicit copy assignment operator for 'Envoy::Network::SocketOptionName' first required here
  info.name_ = optname_;
             ^
1 error generated.
INFO: Elapsed time: 2620.084s, Critical Path: 126.35s
INFO: 4237 processes: 154 internal, 4083 local.
FAILED: Build did NOT complete successfully
ERROR:root:Building fuzzers failed.
```

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
Fixes an oss-fuzz Envoy compilation error:
```
Step envoyproxy#3 - "compile-afl-address-x86_64": Execution platform: @local_config_platform//:host
Step envoyproxy#3 - "compile-afl-address-x86_64": In file included from source/extensions/filters/http/ext_proc/client_impl.cc:1:
Step envoyproxy#3 - "compile-afl-address-x86_64": In file included from ./source/extensions/filters/http/ext_proc/client_impl.h:11:
Step envoyproxy#3 - "compile-afl-address-x86_64": �[1m./source/common/grpc/typed_async_client.h:38:3: �[0m�[0;1;31merror: �[0m�[1mdefinition of implicit copy assignment operator for 'AsyncStream<envoy::service::ext_proc::v3::ProcessingRequest>' is deprecated because it has a user-declared copy constructor [-Werror,-Wdeprecated-copy]�[0m
Step envoyproxy#3 - "compile-afl-address-x86_64":   AsyncStream(const AsyncStream& other) = default;
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0;1;32m  ^
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0m�[1msource/extensions/filters/http/ext_proc/client_impl.cc:34:11: �[0m�[0;1;30mnote: �[0min implicit copy assignment operator for 'Envoy::Grpc::AsyncStream<envoy::service::ext_proc::v3::ProcessingRequest>' first required here�[0m
Step envoyproxy#3 - "compile-afl-address-x86_64":   stream_ = client_.start(*descriptor, *this, options);
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0;1;32m          ^
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0m1 error generated.
```

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
@disconnect3d
Copy link
Contributor Author

Fixed DCO and removed the default assignment operators

@ggreenway
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18919 (comment) was created by @ggreenway.

see: more, trace.

@ggreenway
Copy link
Contributor

@yanavlasov please review

@disconnect3d
Copy link
Contributor Author

@yanavlasov ping :)

@phlax
Copy link
Member

phlax commented Dec 16, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18919 (comment) was created by @phlax.

see: more, trace.

@adisuissa
Copy link
Contributor

I'm not sure if these changes are needed at the moment.
@disconnect3d WDYT?

@disconnect3d
Copy link
Contributor Author

disconnect3d commented Dec 16, 2021

@adisuissa We suggest those changes to fix the oss-fuzz build of Envoy which seems to be broken for almost a year. Or at least the https://oss-fuzz-build-logs.storage.googleapis.com/index.html#envoy page reports that the last successful build was on 31/01/2021 and all the builds that happen now fail due to the issues that we try to fix in this PR.

The initial set of changes sent were enough to fix that build as we tested it internally in Trail of Bits.

So it seems we either need those changes OR a reconfiguration of oss-fuzz build to keep Envoy being fuzzed (or should I say: start being fuzzed again?).

@adisuissa
Copy link
Contributor

To the best of my knowledge, they were broken before, and then we added #18865 and #18896 to address some of the issues, and also removed AFL (google/oss-fuzz#6891) to get the build working.

The build now partially works (the fuzzers are built, but the validation step is still failing as can be seen in the recent logs).

@yanavlasov
Copy link
Contributor

From the logs of the most recent build it looks like compilation is now successful, but then there are ASAN failures in the gtest framework.

@adisuissa
Copy link
Contributor

From the logs of the most recent build it looks like compilation is now successful, but then there are ASAN failures in the gtest framework.

Yes, this is due to part of the code compiled with ASAN and part of it without, see: https://github.com/google/oss-fuzz/blob/9d0a2b953a9ae4e8c6a853dcbd7053081b12a322/projects/envoy/build.sh#L83.

After solving this, there is another issue (timeout during the check_build step), that I'm trying to solve at the moment.

@disconnect3d
Copy link
Contributor Author

disconnect3d commented Jan 3, 2022

After solving this, there is another issue (timeout during the check_build step), that I'm trying to solve at the moment.

We ran things on our private server so we didn't get any timeouts.

Yes, this is due to part of the code compiled with ASAN and part of it without, see: https://github.com/google/oss-fuzz/blob/9d0a2b953a9ae4e8c6a853dcbd7053081b12a322/projects/envoy/build.sh#L83.

As far as I remember we also modified this on our end, but it was not needed for the initial build but for some of the harnesses (otherwise they crashed at startup, most likely due to not everything being sanitized properly).

Anyway, it seems that Envoy oss-fuzz builds are up again (and we are now getting crash reports), so I guess this PR can be closed :)

image
(screenshot via https://oss-fuzz-build-logs.storage.googleapis.com/index.html#envoy)

@jmarantz jmarantz dismissed yanavlasov’s stale review January 6, 2022 14:56

issue was addressed.

@jmarantz jmarantz merged commit 3865cba into envoyproxy:main Jan 6, 2022
@adisuissa
Copy link
Contributor

Not sure if this PR was supposed to be merged or closed.
I'll take a look at the fuzz-build results tomorrow and if it fails, we'll need to revert this PR.

joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
This PR fixes two compilation error that are happening in the oss-fuzz build of Envoy. Note that the latest oss-fuzz build errors out with only the error related to AsyncStream but AFAIR when it is fixed, a different compilation error has to be fixed which I fixed in the first commit that changes the socket.h file.


Fixes an oss-fuzz Envoy compilation error:
```
Execution platform: @local_config_platform//:host
In file included from source/common/network/socket_option_impl.cc:1:
In file included from ./source/common/network/socket_option_impl.h:6:
In file included from ./envoy/network/listen_socket.h:14:
./envoy/network/socket.h:26:3: error: definition of implicit copy assignment operator for 'SocketOptionName' is deprecated because it has a user-declared copy constructor [-Werror,-Wdeprecated-copy]
  SocketOptionName(const SocketOptionName&) = default;
  ^
source/common/network/socket_option_impl.cc:52:14: note: in implicit copy assignment operator for 'Envoy::Network::SocketOptionName' first required here
  info.name_ = optname_;
             ^
1 error generated.
INFO: Elapsed time: 2620.084s, Critical Path: 126.35s
INFO: 4237 processes: 154 internal, 4083 local.
FAILED: Build did NOT complete successfully
ERROR:root:Building fuzzers failed.
```

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>

* types_async_client.h: fix deprecated defaulted copy ctor

Fixes an oss-fuzz Envoy compilation error:
```
Step envoyproxy#3 - "compile-afl-address-x86_64": Execution platform: @local_config_platform//:host
Step envoyproxy#3 - "compile-afl-address-x86_64": In file included from source/extensions/filters/http/ext_proc/client_impl.cc:1:
Step envoyproxy#3 - "compile-afl-address-x86_64": In file included from ./source/extensions/filters/http/ext_proc/client_impl.h:11:
Step envoyproxy#3 - "compile-afl-address-x86_64": �[1m./source/common/grpc/typed_async_client.h:38:3: �[0m�[0;1;31merror: �[0m�[1mdefinition of implicit copy assignment operator for 'AsyncStream<envoy::service::ext_proc::v3::ProcessingRequest>' is deprecated because it has a user-declared copy constructor [-Werror,-Wdeprecated-copy]�[0m
Step envoyproxy#3 - "compile-afl-address-x86_64":   AsyncStream(const AsyncStream& other) = default;
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0;1;32m  ^
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0m�[1msource/extensions/filters/http/ext_proc/client_impl.cc:34:11: �[0m�[0;1;30mnote: �[0min implicit copy assignment operator for 'Envoy::Grpc::AsyncStream<envoy::service::ext_proc::v3::ProcessingRequest>' first required here�[0m
Step envoyproxy#3 - "compile-afl-address-x86_64":   stream_ = client_.start(*descriptor, *this, options);
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0;1;32m          ^
Step envoyproxy#3 - "compile-afl-address-x86_64": �[0m1 error generated.
```

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>

* socket.h: delete default assignment operator

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>

* typed_async_client.h: remove default assignment op

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
Signed-off-by: Josh Perry <josh.perry@mx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants