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

Max allocation on invalid string input #92

Open
Thomasvdam opened this issue Oct 11, 2024 · 5 comments
Open

Max allocation on invalid string input #92

Thomasvdam opened this issue Oct 11, 2024 · 5 comments

Comments

@Thomasvdam
Copy link

First of all thanks for all the work on this library, it's been incredibly useful to us!

We ran into an issue where AssemblyScript complained that we were allocating too much memory (Allocation too large in ~lib/rt/itcms.ts(261:31)) and after some digging we found that the error originated in the string parsing when a value was incorrectly typed as a string but the received JSON value was a single digit number.

While the fault ultimately was with us not typing the expected JSON correctly we think it might be useful to have the string deserialiser do some sanity checks on the input and abort with a more helpful error message.

If this is something you'd be interested in adding to the library we can work on it and submit a PR. :)

Reproduction scenario in a test file:

@json
class IncorrectlyTyped {
  value: string;
}
describe("Should not allocate max memory on invalid input", () => {
  expect(JSON.stringify(JSON.parse<IncorrectlyTyped>('{"value":0}'))).toBe(
    JSON.stringify(<IncorrectlyTyped>{ value: "" }),
  );
});

Outputs:

abort: Allocation too large in ~lib/rt/itcms.ts(261:31)
Error: failed to run main module `build/test.spec.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0: 0x4b7a - <unknown>!<wasm function 387>
           1: 0x1839 - <unknown>!<wasm function 59>
           2: 0xdd9b - <unknown>!<wasm function 513>
           3: 0xe1a9 - <unknown>!<wasm function 517>
           4: 0xe4e6 - <unknown>!<wasm function 518>
           5: 0x161bc - <unknown>!<wasm function 606>
           6: 0x16748 - <unknown>!<wasm function 607>
           7: 0x16931 - <unknown>!<wasm function 611>
           8: 0x174a3 - <unknown>!<wasm function 629>
           9: 0x19738 - <unknown>!<wasm function 655>
          10: 0x1afa9 - <unknown>!<wasm function 656>
          11: 0x48ff - <unknown>!<wasm function 383>
    2: exit with invalid exit status outside of [0..126)
undefined:1
@JairusSW
Copy link
Owner

@Thomasvdam, coming down the pipeline :)
@mattjohnsonpint, can I make this work-related?

@JairusSW
Copy link
Owner

@Thomasvdam, i'll release this in the next version. Should be dropping by end of week

@Thomasvdam
Copy link
Author

Amazing, thank you for picking this up on such a short notice!

@JairusSW
Copy link
Owner

@Thomasvdam, for safety, I do recommend setting a default value for every type, so you should go ahead and do that. When parsing an incorrect type, it should just ignore it if not assignable.
I have implemented JSON.parseSafe() which will throw, but it is likely to have breaking changes. Eg. when implementing #58

@Thomasvdam
Copy link
Author

@Thomasvdam, for safety, I do recommend setting a default value for every type, so you should go ahead and do that. When parsing an incorrect type, it should just ignore it if not assignable.

I'm not quite sure what you mean by this, I get errors on both JSON.parse and JSON.parseSafe for classes with or without default values:

@json
class InvalidAttributeWithDefault {
	value: string = "";
}

@json
class InvalidAttribute {
	value!: string;
}

Regardless, JSON.parseSafe is already super helpful so thanks a lot for that! We're looking forward to the improved error handling, that's such a great feature we don't mind the API changing when it lands 😄

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

2 participants