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

SEGV in xsRun.c on eval of large (~200KB) string #399

Closed
dckc opened this issue Aug 10, 2020 · 8 comments
Closed

SEGV in xsRun.c on eval of large (~200KB) string #399

dckc opened this issue Aug 10, 2020 · 8 comments

Comments

@dckc
Copy link
Contributor

dckc commented Aug 10, 2020

I got a SEGV at this line when trying to eval a large (~200KB) string:

mxNextCode(mxStack->value.integer ? (txS4)index : (txS4)index + offset)

To isolate the problem, I put the big string of js in main.js in the hello-world moddable SDK example and I get bug1/main.js:14: error: buffer overflow! I tried allocating more space (using creation stuff), but the symptoms were the same. The goal is to eval the string, but I can't even get it to compile.

Details are in https://gist.github.com/dckc/0f6c74cd669c37079993c0d724f2e6da

cc @warner @michaelfig
context: Agoric/agoric-sdk#1299 , Agoric/agoric-sdk#1407

@erights
Copy link

erights commented Aug 10, 2020

@phoddie We're blocked on this. Please advise. Are we doing something wrong? Any workaround until it is fixed?

Thanks.

@dckc
Copy link
Contributor Author

dckc commented Aug 10, 2020

I'm actually using a version of the SDK from Jun 5 62c7f1e with some platform tweaks.

I hope to try again with the latest moddable SDK soon...

@phoddie
Copy link
Collaborator

phoddie commented Aug 10, 2020

The exception is generated from fxUTF8Buffer. A quick look suggests that the problem is that the source code contains a single element that exceeds the size of the parse buffer in XS, probably something in your sourceBundle.

@dckc was on the right track with increasing the parser buffer size. However, the manifest adjusts the buffer for the runtime, not the tools. The buffer for the tools is 32 KB, but two of your strings are well over that.

Increase buffers in the tools isn't a concern, so we will look into changing the default. In the meantime, you can build by forcing bufferSize to 128 * 1024 at the top of fxInitializeParser prior to calling fxNewParserChunk.

void fxInitializeParser(txParser* parser, void* console, txSize bufferSize, txSize symbolModulo)
{
c_memset(parser, 0, sizeof(txParser));
parser->first = C_NULL;
parser->console = console;
parser->buffer = fxNewParserChunk(parser, bufferSize);

@dckc
Copy link
Contributor Author

dckc commented Aug 10, 2020

@dckc was on the right track with increasing the parser buffer size. However, the manifest adjusts the buffer for the runtime, not the tools. ...

runtime (i.e. eval) is the goal.

I'm tried to reproduce the problem with a small example using eval, but the symptoms go away.

(I updated https://gist.github.com/dckc/0f6c74cd669c37079993c0d724f2e6da with details.)

good news: I just realized xsbug works with our x-cli-lin headless platform. I don't know why I thought it didn't. This could be really handy in diagnosing the larger problem.

@dckc
Copy link
Contributor Author

dckc commented Aug 10, 2020

The long string seems to be a red herring. The crash comes near Object.freeze; in particular, this expedient hack:

// ISSUE: harden compat? XXX IOU issue number
function harden(x) {
  return Object.freeze(x, true);
}

EDIT the crash actually came a little bit after that function... when the 200KB string was evaluated in a new compartment.

@erights
Copy link

erights commented Aug 10, 2020

We should NOT use that hacky replacement for harden. We should use the harden from the SES-shim monorepo. If for some reason it does not work out of the box, we need to fix it. Its semantics are different that the two argument Object.freeze in important way.

We should never use the two-argument form of Object.freeze as it enables attacks. Our repairs for running on XS should in fact replace the builtin Object.freeze with a wrapper that has only a single argument.

@phoddie
Copy link
Collaborator

phoddie commented Aug 10, 2020

If there is an open issue about large strings, please let me know. Otherwise, let's close this issue out.

@erights: With regard to Object.freeze we have always said are more than happy to replace it with a standard solution, when one becomes available.

Since nearly all of Moddable's use of Object.freeze occur at build time (e.g. preload), perhaps a a simple short-term solution would be to only enable the non-standard behavior during the preload phase of execution. Would that address Agoric's needs?

@dckc
Copy link
Contributor Author

dckc commented Aug 11, 2020

I'm definitely getting a SEGV from somewhere. But until I know more about where, yes, let's close this.

@dckc dckc closed this as completed Aug 11, 2020
mkellner pushed a commit that referenced this issue Aug 21, 2020
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

No branches or pull requests

3 participants