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

Ensure Platform globals are freed on error #2685

Merged
merged 7 commits into from
Jul 15, 2022
Merged

Ensure Platform globals are freed on error #2685

merged 7 commits into from
Jul 15, 2022

Conversation

anrossi
Copy link
Contributor

@anrossi anrossi commented May 6, 2022

Description

Free platform globals in error path. Fixes #2683

Testing

SpinQuic is how this was found, and the fix was validated against the SpinQuic scenario which found it.

Documentation

N/A.

@anrossi anrossi requested a review from a team as a code owner May 6, 2022 22:42
@nibanks
Copy link
Member

nibanks commented May 7, 2022

By default, AFAIU, the C language always auto initializes globals with zero. I don't think PR is doing anything,

@anrossi
Copy link
Contributor Author

anrossi commented May 9, 2022

I'm not so sure of that; in the test failure that inspired me to make this fix, these globals were just addresses and non-NULL, which caused the test to crash when trying to free them. Perhaps it was from a previous run, but the globals should be set to NULL at the end of the run, so I don't think it is that.

@thhous-msft
Copy link
Contributor

Globals are definitely 0 initialized on program start. So I'd be more inclined to look at a bug with a previous run not resetting everything correctly.

@nibanks
Copy link
Member

nibanks commented May 9, 2022

How about try writing a test that validates the initial conditions between resets?

@anrossi
Copy link
Contributor Author

anrossi commented May 9, 2022

I'm first adding some debug asserts to ensure we catch when these are not-NULL. I'll brainstorm some more on how to write a test for this. Seems spinquic might be exercising this scenario quite a bit, and is likely the best place to catch it.

nibanks
nibanks previously approved these changes Jun 10, 2022
@nibanks
Copy link
Member

nibanks commented Jun 10, 2022

@anrossi there are a couple spinquic failures. Can you double check to see if they are hitting your new asserts?

@anrossi anrossi changed the title Ensure Platform globals are zero-initialized. Ensure Platform globals are freed on error Jul 15, 2022
@anrossi anrossi merged commit 74c6346 into main Jul 15, 2022
@anrossi anrossi deleted the anrossi/fix-2683 branch July 15, 2022 22:20
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.

SpinQuic Crash due to uninitialized memory
3 participants