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

kernel: close streams after internal exception #5896

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

... then rethrow.

Otherwise we may end up with references to input/output stream objects on the stack that are invalid.

I am actually not sure why this worked so far w/o causing us crashes in various situations. I think I reasoned this through at some point in the distant past and convinced myself the code is fine as it is, but I don't see it anymore. In any case, even if for some reason it was OK after all, I feel it is better to explicitly deal with this.

Diffs best viewed with "ignore whitespace" turned on.

... then rethrow.

Otherwise we may end up with references to input/output stream
objects on the stack that are invalid.
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 6, 2025
@ChrisJefferson
Copy link
Contributor

If you are going to try/catch in these functions, I think you need to make all the variables volatile so they are compatible with longjmp/setjmp? (could do it selectively, but probably easiest to just do everything)

@fingolfin
Copy link
Member Author

In most of the changes, register clobbering is no problem: if we longjmp (which may result in clobbered data) we go into the GAP_CATCH block, which in all but one cases in this PR access a single local variable input, which is > 32kb and thus never stored in a register ;-), and then we GAP_THROW so control flow leaves and anything after the GAP_CATCH block is irrelevant.

There is one exception: in FuncREAD_STREAM_LOOP the GAP_CATCH block only sets a local BOOL rethrow to TRUE and then proceeds. Interestingly GCC nagged me here into adding volatile to the context object, which is not really used afterwards? We do use input and output but those are too bigger for clobbering. That leaves oldPrintObjState which should get a volatile (but GCC did not complain about it.. sigh)

@fingolfin
Copy link
Member Author

@ChrisJefferson thoughts?

@ChrisJefferson
Copy link
Contributor

Is "large so never stored in a register" sufficient? This being C, my assumption is the compiler will mess up the code at high optimisation, just because it can?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) kind: bug Issues describing general bugs, and PRs fixing them release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants