Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Better handle errors from overlong arrays or overlarge pointers #2417

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Sep 25, 2019

This PR puts better error-handling around uses of numbers that are possibly too large in the decoder. Two new DecoderError types, OverlongArraysAndStringsNotImplementedError and OverlargePointersNotImplementedError, have been added. The latter is used when a pointer is too large to safely fit in a number, the former when a length is. These both have "NotImplemented" in the name to make it clear that, yeah, I guess such a thing could exist, but we don't support it.

The old PointerTooLargeError is gone, along with the check for it, as it's been superseeded by OverlargePointersNotImplementedError, and the check for it was only ever there to prevent problems with toNumber(). However, the old OverlongArrayOrStringError, and the check for it, is still there, though it's been renamed OverlongArrayOrStringStrictModeError. It's still there because it has a purpose beyond preventing problems with toNumber(); it's there to make sure the decoder doesn't accidentally DOS itself should it, when decoding a possibly-ambiguous event, mistake a really large number (but not so large as to not safely fit in a number) for a length. So that's still around.

Also, checks for number safety have been added in read/storage.ts and read/bytes.ts, meaning two new types of read errors have been added, ReadErrorStorage and ReadErrorBytes. Unfortunately if you get one of these the message may not be as informative as I'd like, but, oh well. (This is especially true of ReadErrorBytes, which, due to the way things are currently organized, can't even tell you whether the read error ocurred in memory or in calldata.) In any case it's not like I expect this sort of thing to come up much at all; there's a reason I didn't bother with these errors before. Also, reads in decode/storage.ts have now been wrapped in try/catches as a result.

This PR also gets rid of a ton of circular dependencies that existed in @truffle/codec, because at least one of them was causing problems. So, a number of types and functions have been moved around, but ultimately it all worked out. Well, almost -- there's one circular dependency still remaining, and it's one introduced in this PR. I've leaving it in because it's not causing any problems at the moment and it looks like getting rid of it may be difficult. I'm planning to address that in a separate PR.

(Edit: OK, actually I added another commit to this PR, addressing it. Read on for the original description of the circularity, and see the following comment for how I resolved it.)

That circular dependency is this:

  1. format/errors.ts depends on types/storage.ts, because the new ReadErrorStorage includes a Range as part of the error.
  2. types/storage.ts depends on format/values.ts, because a Slot may contain an ElementaryValue as a mapping key.
  3. format/values.ts depends on format/errors.ts, because a Result may be an ErrorResult.

The best solution to this is presumably to split Values into Values and Results, but that's going to be a pain to do, so like I said, I'm saving it for a different PR. Once that's done, though, then you'll have Result > ErrorResult > DecoderError > ReadErrorStorage > Range > Slot > ElementaryValue with no circularity.

@coveralls
Copy link

coveralls commented Sep 25, 2019

Coverage Status

Coverage remained the same at 70.347% when pulling b18b924 on ridley-array into 96330bd on next.

@haltman-at
Copy link
Contributor Author

OK, I've gone and removed this final circular dependency. The solution is pretty inelegant, but it works. It involves defining ElementaryValue, and all elementary Value types, in a new file, format/elementary.ts. Then format/values.ts imports this, and does not define elementary values itself but just re-exports the definitions in its own namespace. So most things can continue to use the same names as always; no great renaming necessary. However, types/storage.ts now imports elementary.ts instead of values.ts, thus preventing the circularity.

@haltman-at haltman-at merged commit e864610 into next Sep 26, 2019
@haltman-at haltman-at deleted the ridley-array branch September 26, 2019 21:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants