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

Make the deps_log parser report corrupted files instead of crashing. #2489

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

digit-google
Copy link
Contributor

The parser used asserts to verify the consistency of the deps log, which resulted in crashes in production. Replace them with correct checks and return conditions.

This allows the parser to survive and report corrupted files, and Ninja to ignore the latter.

  • Add related unit-test.

Fixes #2472
Fixes #2309

@jhasse jhasse added this to the 1.13.0 milestone Sep 2, 2024
@digit-google digit-google force-pushed the safer-depslog-parser branch 2 times, most recently from bea6f78 to ebbef6c Compare September 4, 2024 09:52
Call fopen() with "wb" instead of "w" to avoid translating
\n into \r\n on Windows, based on the machine's configuration.

This prevents random test breakages on Windows/x64 CI builders
that do not happen on Windows/arm64 or other operating systems.
The parser used asserts to verify the consistency of the
deps log, which resulted in crashes in production. Replace
them with correct checks and return conditions.

This allows the parser to survive and report corrupted
files, and Ninja to ignore the latter.

+ Add related unit-test.

Fixes ninja-build#2472
Fixes ninja-build#2309
@digit-google
Copy link
Contributor Author

The Windows/x64 CI builder was failing, but not the Windows/arm64 one!, due to a bug in DiskInterface::WriteFile().
I fixed this in a separate commit for this PR, but it's been a wild ride, since I couldn't reproduce this locally. Damn.

@jhasse jhasse merged commit fc2f81e into ninja-build:master Oct 4, 2024
11 checks passed
@jhasse
Copy link
Collaborator

jhasse commented Oct 4, 2024

Awesome stuff, thank you very much for this :)

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.

Ninja triggers GLIBCXX assertion and crashes Consistent ninja crash when my build folder has a specific path
2 participants