Skip to content

quiche: fix SpdyStringUtilsTest ASAN failure and disable quic_mem_slice_storage_test #10268

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 5, 2020
Merged

quiche: fix SpdyStringUtilsTest ASAN failure and disable quic_mem_slice_storage_test #10268

merged 4 commits into from
Mar 5, 2020

Conversation

danzh2010
Copy link
Contributor

@danzh2010 danzh2010 commented Mar 5, 2020 •
edited
Loading

Signed-off-by: Dan Zhang danzh@google.com

Exclude the test from the test suit because it breaks coverage test. Will add it back once fixed.
Fix an asan failure in HexDecodeToUInt32():

ource/extensions/quic_listeners/quiche/platform/string_utils.cc:92:16: runtime error: load of misaligned address 0x7ffed8abe381 for type 'const uint32_t' (aka 'const unsigned int'), which requires 4 byte alignment

@danzh1989
disable test
f3b515d 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Contributor Author

/assign @mattklein123

mattklein123
mattklein123 previously approved these changes Mar 5, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mattklein123
Copy link
Member

Oops can you fix format?

/wait

@danzh1989
fix asan failure
9429ca9 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010 danzh2010 requested a review from alyssawilk as a code owner March 5, 2020 19:46
@danzh2010 danzh2010 changed the title quiche: disable quic_mem_slice_storage_test quiche: fix SpdyStringUtilsTest ASAN failure and disable quic_mem_slice_storage_test Mar 5, 2020
mattklein123
mattklein123 previously approved these changes Mar 5, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

jmarantz
jmarantz reviewed Mar 5, 2020
View reviewed changes
source/extensions/quic_listeners/quiche/platform/string_utils.cc
@@ -89,7 +89,9 @@ bool HexDecodeToUInt32(absl::string_view data, uint32_t* out) {
std::string byte_string = absl::HexStringToBytes(data_padded);

ASSERT(byte_string.size() == 4u);
*out = ntohl(*reinterpret_cast<const uint32_t*>(byte_string.c_str()));
uint32_t bytes;
memcpy(&bytes, byte_string.data(), byte_string.length());
Copy link
Contributor

Choose a reason for hiding this comment

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

per discussions about memcpy issues can you change the above ASSERT to a RELEASE_ASSERT, or, use std::min(byte_string.length(), sizeof(bytes) as the 3rd arg for memcpy.

That way this will be obviously correct at a glance without worrying about what HexStringToBytes might return.

Copy link
Contributor

Choose a reason for hiding this comment

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

or you can use MemBlockBuiilder in source/common/common/mem_block_builder.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@danzh1989
release assert
ee45792 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh1989
fix RELEASE_ASSERT
2730c4f 
Signed-off-by: Dan Zhang <danzh@google.com>
mattklein123
mattklein123 reviewed Mar 5, 2020
View reviewed changes
source/extensions/quic_listeners/quiche/platform/string_utils.cc
@@ -88,8 +88,10 @@ bool HexDecodeToUInt32(absl::string_view data, uint32_t* out) {

std::string byte_string = absl::HexStringToBytes(data_padded);

ASSERT(byte_string.size() == 4u);
*out = ntohl(*reinterpret_cast<const uint32_t*>(byte_string.c_str()));
RELEASE_ASSERT(byte_string.size() == 4u, "padded dtat is not 4 byte long.");
Copy link
Member

Choose a reason for hiding this comment

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

s/dtat/data

Can you fix this in your next change? If this passes I would like to unwedge CI. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK

@mattklein123
Copy link
Member

Coverage is now failing with

test/common/http/http2/metadata_encoder_decoder_test.cc:356: Failure
Death test: nghttp2_session_send(session_)
    Result: died but not with expected error.
  Expected: contains regular expression "No payload remaining to pack into a METADATA frame."
Actual msg:
[  DEATH   ] Turning perftools heap leak checking off
[  DEATH   ] 
Stack trace:
  0x64f9017: Envoy::Http::Http2::MetadataEncoderDecoderDeathTest_PackTooManyFrames_Test::TestBody()
  0x10ddbeb6: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
  0x10dc83f1: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x10db2cc2: testing::Test::Run()
  0x10db37c8: testing::TestInfo::Run()
... Google Test internal frames ...

[  FAILED  ] MetadataEncoderDecoderDeathTest.PackTooManyFrames (7829 ms)

Which I think is from a recent @mkbehr PR. I'm going to force merge so we can make progress on fixing coverage.

@mattklein123 mattklein123 merged commit 1c40089 into envoyproxy:master Mar 5, 2020
@mkbehr
Copy link
Contributor

mkbehr commented Mar 6, 2020

Oops! Sent out #10279 to fix coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@jmarantz jmarantz jmarantz approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

Assignees

@jmarantz jmarantz

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danzh2010 @mattklein123 @mkbehr @jmarantz @danzh1989