bug: fix for memory issue

[segfault-magnet]

closes: #1058
codeveloped with @Salka1988
Special tnx to @nedsalk for brainstorming with us.

The issue from the example

There was a memory problem in the example but not for the reasons listed in the issue. The issue (quoting the audit) says:

The reason for this issue is that the decoder supports zero-sized types which do not result in progress during parsing. Therefore, the parser might run for a long time and eventually run out of memory.

Units in Sway are encoded as 1WORD of zeroes. So the given example didn't experience the zero-sized-type issue. What it did experience is a caveat of how we decoded multiple tokens of the same type (by cloning the param type). This is now replaced with a std::iter::repeat which will copy (as needed) the reference to a single ParamType solving the memory issue (by only having 1 reference to the param type at any moment).

Real zero-sized types

A real zero-sized type would be an empty struct or an empty string array (i.e. str[0]).

Enums don't come into play since empty enums cannot be instantiated. Also, an Enum with a ZST variant would have been at least 1 WORD since it encodes the discriminant, making it not a ZST.

You also cannot define an empty tuple (as far as we're aware).

Empty structs could also eventually be used as markers (e.g. Tcp<Connected> where struct Connected{}).

The solution

The decoder now can be configured to accept two upper limits: one for the maximum tolerable depth, the second for the maximum tokens allowed.

The depth limit protects our stack (think call-frames and recursions) against types such as these:

struct Parent {
   child1: Child1 { 
      child2: Child2 {
          .... 
            child3: Child3 { ...
                  child7812341: Child7812341
      }
}

The whole structure would be zero-sized and contain so many levels of nested zero-size structs that we might run out of stack trying to decode it.

The second limit (max_tokens) protects against:

• collections of ZSTs
• structures with endless fields of ZSTs
• basically any other combination we could conceive of.

We've set the defaults to 45 (levels of nesting) and 10k max tokens.

The user can tweak the decoder used for contract and script calls. Tests have been added and the docs updated.

Checklist

• I have linked to any relevant issues.
• I have updated the documentation.
• I have added tests that prove my fix is effective or that my feature works.
• I have added necessary labels.
• I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
• I have requested a review from the relevant team or maintainers.

[digorithm]

Alright, so correct if I'm wrong: this is solving the memory issue only for collections and not the other cases you mentioned in the description related to other ZSTs, right? If that's the case, the title should mention it, and the memory issue is partially solved (maybe create a separate issue for the follow-up work).

As for handling the other ZSTs... banning them would probably not be a good approach. I'd rule this one out.

Defining a timeout mechanism that's mostly implicit and only explicit when the user needs it (e.g., they just experienced the OOM issue and need to act on it) sounds like a good idea.

Now... onto defining the limits, yeah... token based seems weird. Not sure. Time-based seems like a safe bet. I might be off here, but this type of decoding seems like it's either very fast (e.g., by and large, less than a handful of seconds most of the time) or something ill-defined or malicious that's straight-up undesirable that would take more than, say 30 seconds. This could cover the vast majority of cases.

Then, in case of slower hardware struggling to decode reasonable and valid data for whatever reason, this user will see a timeout error, and the error should direct the user to increase the configurable timeout limit. What do y'all think?

[segfault-magnet]

Alright, so correct if I'm wrong: this is solving the memory issue only for collections and not the other cases you mentioned in the description related to other ZSTs, right?

Right.

If that's the case, the title should mention it, and the memory issue is partially solved (maybe create a separate issue for the follow-up work).

I'll change the title. Will create an issue as soon as we agree on further steps.

Defining a timeout mechanism that's mostly implicit and only explicit when the user needs it (e.g., they just experienced the OOM issue and need to act on it) sounds like a good idea.

Now... onto defining the limits, yeah... token based seems weird. Not sure. Time-based seems like a safe bet. I might be off here, but this type of decoding seems like it's either very fast (e.g., by and large, less than a handful of seconds most of the time) or something ill-defined or malicious that's straight-up undesirable that would take more than, say 30 seconds. This could cover the vast majority of cases.

Didn't play around with it but my thinking was that, due to our recursive approach to decoding, every nesting level will incur a call frame penalty to the stack along with any other data we may leave on the stack. Some of the recursions might be tail-optimized, but I'm not sure all are.

If the attack was in the form of a struct with a lot of fields, that isn't a problem since we'd be freeing and reserving the stack during the decoding.

But if the attack was in the form of an enormously nested struct, then for each nesting level we'd go deeper in the recursion. This might cause us to hit the stack limit of the thread.

So limiting decoding by elapsed time might prove finicky, how fast will the recursion burrow, i.e. how loaded is the CPU at the moment? The user might be happy with a limit in normal times, only to cause him an OOM if the attack happened while his CPU load was down.

Not sure that untangling the recursion would help by that much either since we need to keep state for every level.

I'll play around, try and recreate what this attack might look like, and get some numbers.

Ideally, we'd limit by time and memory used, but I'm not sure how practical this is to implement.

[digorithm]

Yeah, it sounds like we gotta do a spike to iron out these details.

[Dentosal]

The whole structure would be zero-sized and contain so many levels of nested zero-size structs that we might run out of stack trying to decode it.

Typically you wouldn't run out of stack space, since the parsed structure would be placed into the heap. Then you could have a memory size limit, similar to what e.g. regex crate does.

[segfault-magnet]

The whole structure would be zero-sized and contain so many levels of nested zero-size structs that we might run out of stack trying to decode it.

Typically you wouldn't run out of stack space, since the parsed structure would be placed into the heap. Then you could have a memory size limit, similar to what e.g. regex crate does.

The decoding process is recursive, meant it in the sense that we'd recurse until there is no more stack left. I'm not sure all of our decoding recursions can be tail-call optimized.

I made the PR a draft, my current idea is not to go for a time check but rather let the user configure optionally additional security measures for analyzing a type before even attempting the decoding.

For example: "the type mustn't be deeper than N" or "structs/enums can have at max N fields/variants" and such.

That along with the ZST collections should prove enough configurability so that users might decode from untrusted schemas.

P.S. Tnx for the regex link, I'll look at its impl more closely.

[iqdecay]

LGTM!

[digorithm]

I love the new encoding/decoding session; we badly needed that. I just left a couple of grammar nits to improve flow/clarity.

I'm still going through the actual implementation, but I'd like to send these suggestions now rather than later.

[digorithm]

Thanks for the patience; this was a big one. The new tests helped me a lot to understand these changes. Great job, dude!