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

WIP: Prevent segfaults related to infinite marshal recursion #198

Closed
wants to merge 7 commits into from

Conversation

seanmakesgames
Copy link
Contributor

Creates a marshal stackdepth option, allowing protection against exceptional object structures. See testcases for examples.
Been a while since I've made changes in here, would love some critical eyes on the code.

@seanmakesgames
Copy link
Contributor Author

After v8 ver upgrade, max data slots seem to have reduced. Need to refactor our usage and allocate some memory to track our runtime state per isolate.

@seanmakesgames
Copy link
Contributor Author

Also, @SamSaffron the locale test you added is failing on my machine on the main branch. Is this expected?
darwin 10.15, ruby 2.6.5

@SamSaffron
Copy link
Collaborator

@seanmakesgames error is kind of expected until you update libv8-node, try installing latest and updating lockfile etc.

@SamSaffron
Copy link
Collaborator

I need some time to think about this, can you think of a way of introducing minimal levels of safety here without making this configurable? Maybe we just limit at 1000 depth unconditionally using a const or something like that and the patch can be simpler?

@seanmakesgames
Copy link
Contributor Author

@seanmakesgames error is kind of expected until you update libv8-node, try installing latest and updating lockfile etc.

Got it. Thanks. I figured the dependency file would have forced the upgrade when I ran bundle.

@seanmakesgames
Copy link
Contributor Author

I need some time to think about this, can you think of a way of introducing minimal levels of safety here without making this configurable? Maybe we just limit at 1000 depth unconditionally using a const or something like that and the patch can be simpler?

I can definitely put that together. For my uses I need 100. :)
I'll put together a few different PRs.

@seanmakesgames
Copy link
Contributor Author

@seanmakesgames error is kind of expected until you update libv8-node, try installing latest and updating lockfile etc.

Got it. Thanks. I figured the dependency file would have forced the upgrade when I ran bundle.

I deleted my lock file and rebundled and am still having the error. What version of libv8-node am I looking for here?
Gemfile.lock.txt

@seanmakesgames
Copy link
Contributor Author

@seanmakesgames error is kind of expected until you update libv8-node, try installing latest and updating lockfile etc.

Got it. Thanks. I figured the dependency file would have forced the upgrade when I ran bundle.

I deleted my lock file and rebundled and am still having the error. What version of libv8-node am I looking for here?
Gemfile.lock.txt

This was fixed by #200

@seanmakesgames
Copy link
Contributor Author

I need some time to think about this, can you think of a way of introducing minimal levels of safety here without making this configurable? Maybe we just limit at 1000 depth unconditionally using a const or something like that and the patch can be simpler?

@SamSaffron I just tried to implement this with a fixed depth -- but I can't without #201 because we are at data slot max already: 4/4.

I need flags like MEM_SOFTLIMIT_VALUE and MEM_SOFTLIMIT_REACHED to be able to track the stack depth and report it back safely across thread contexts and such. I'll push my WIP -

@seanmakesgames seanmakesgames changed the title Prevent segfaults related to infinite marshal recursion WIP: Prevent segfaults related to infinite marshal recursion May 8, 2021
@seanmakesgames
Copy link
Contributor Author

I put the non-configurable stack max into #202 -- needs 201 to work. Happy to update them however.

@seanmakesgames
Copy link
Contributor Author

implemented by #202

@seanmakesgames seanmakesgames deleted the cycle branch May 11, 2021 14:25
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.

2 participants