Skip to content

Conversation

@supervacuus
Copy link
Collaborator

This is meant to be merged and released with getsentry/sentry-native#1413.

It removes MAX_PATH dependencies where it makes sense (similar in intent to getsentry/breakpad#43). In particular, MAX_PATH will no longer be used for

  • attachment paths
  • retrieving the module filename via GetModuleFileName()
  • retrieving the pipename via GetFileInformationByHandleEx()

While the latter two are only changes in terms of heap buffer handling, the first item required an adaptation of the IPC protocol. There were two options:

  • adapt the entire IPC protocol to support dynamically sized bytestreams (currently, only fixed-sized messages are supported)
  • add only a special handler for dynamically sized attachment paths on the client and serve

I chose the latter option because it introduces the least amount of change with respect to upstream maintenance.

Comment on lines +432 to +433
auto path_buffer =
base::HeapArray<wchar_t>::Uninit(path_length_bytes / sizeof(wchar_t));

Choose a reason for hiding this comment

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

Potential bug: The buffer allocation for path_buffer uses integer division on path_length_bytes without validating its divisibility by sizeof(wchar_t), potentially creating an undersized buffer.
  • Description: In HandleAddAttachmentV2 and HandleRemoveAttachmentV2, the buffer for an attachment path is allocated using path_length_bytes / sizeof(wchar_t). This calculation uses integer division, which truncates the result. If the server receives a path_length_bytes value from a client that is not evenly divisible by sizeof(wchar_t) (2 bytes), the allocated path_buffer will be smaller than path_length_bytes. The subsequent call to LoggingReadFileExactly attempts to read the full path_length_bytes into this undersized buffer, causing a heap buffer overflow. This can lead to memory corruption or a crash of the exception handler server.

  • Suggested fix: Before allocating the buffer in HandleAddAttachmentV2 and HandleRemoveAttachmentV2, add a check to validate that path_length_bytes is evenly divisible by sizeof(wchar_t). If the check fails, log an error and return early to prevent the allocation and subsequent buffer overflow.
    severity: 0.85, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While we can be sure that our untampered client will send bytes that are multiples of wchar_t, since it is IPC, we should protect against malicious or corrupted input. I will add further validation of the incoming message.

@supervacuus supervacuus merged commit 1b4a591 into getsentry Oct 20, 2025
17 checks passed
@supervacuus supervacuus deleted the fix/win/eliminate_max_path branch October 20, 2025 12:21
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.

4 participants