Skip to content

Guard against unsigned overflow. #4093

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

Merged
merged 2 commits into from
Mar 21, 2022

Conversation

adrian-prantl
Copy link

If addr+size overflows (for example because addr is
LLDB_INVALID_ADDRESS) then this check still succeeds because addr+size
will be <= m_local_buffer + m_local_buffer_size).

@adrian-prantl
Copy link
Author

@swift-ci test

@@ -233,6 +233,7 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
if (m_local_buffer) {
auto addr = address.getAddressData();
if (addr >= *m_local_buffer &&
addr < *m_local_buffer + m_local_buffer_size &&

Choose a reason for hiding this comment

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

for other reviewers, this is the non-formatting change

@adrian-prantl
Copy link
Author

@swift-ci test linux

@adrian-prantl
Copy link
Author

@swift-ci test Linux

@adrian-prantl
Copy link
Author

Context: I've seen this crash on an Apple Silicon machine today — I'm sure the root cause is something else, but this check still is wrong.

@shafik
Copy link

shafik commented Mar 18, 2022

Note MathExtras.h has SaturatingAdd(...) with an ResultOverflowed available. Instead of rolling our own this looks like what you want.

@adrian-prantl
Copy link
Author

@swift-ci test

If addr+size overflows (for example because addr is
LLDB_INVALID_ADDRESS) then this check still succeeds because addr+size
will be <= m_local_buffer + m_local_buffer_size).
@adrian-prantl
Copy link
Author

@swift-ci test

@adrian-prantl
Copy link
Author

@swift-ci test linux

@adrian-prantl adrian-prantl merged commit 60ff1e4 into swiftlang:stable/20211026 Mar 21, 2022
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.

3 participants