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

feat: types: apply a max length when decoding events #11054

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Jul 5, 2023

Related Issues

A security report pointed out the unbounded allocation here, which could be problematic. In practice, this is not a problem, since we can trust the FVM not too return dangerously large data, which can in turn trust gas costs to not allow several GiBs of events.

Proposed Changes

Despite that, we might as well apply some limit here. I settled on 6 million, since it seems like a good theoretical limit, given that a single message can't use more than 10 Billion gas (block gas limit), and a single event must cost at least 1750 gas, leading to a theoretical max of 5.7 Million entries.

I'm happy to use something bigger or smaller, this number is only consensus critical for folks actually decoding events, which Lotus doesn't do by default.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@arajasek arajasek requested a review from a team as a code owner July 5, 2023 15:05
@Stebalien
Copy link
Member

Ok, so, this is the maximum number of events as emitted by the FVM when encoded as an array.

  • The minimum gas is more like 14000 gas because that's the base cost of a syscall.
  • However, the max gas we care about here is tipset gas which... doesn't theoretically have a max limit.

6M events per epoch is probably safe, but I'm inclined to simply move DecodeEvents into chain/vm/fvm.go and make it private. It's not designed external use, it's designed for passing data from the FVM to lotus.

@Kubuxu
Copy link
Contributor

Kubuxu commented Jul 6, 2023

Another option would be to not pre-allocate a slice larger than N. If someone passes an enormous blob into there and gets, let's say, 30x expansion, it is bad but not as bad as passing a couple hundred bytes and OOMing.

@jennijuju
Copy link
Member

Seems missing tests

@arajasek
Copy link
Contributor Author

arajasek commented Jul 6, 2023

  • However, the max gas we care about here is tipset gas which... doesn't theoretically have a max limit.

Wait, no it's not? These are the events emitted by a single message, which is bounded by block gas limit?

6M events per epoch is probably safe, but I'm inclined to simply move DecodeEvents into chain/vm/fvm.go and make it private. It's not designed external use, it's designed for passing data from the FVM to lotus.

Seems reasonable, gonna do that, that definitely derisks this from future danger. Okay if I keep the max limit anyway?

@arajasek
Copy link
Contributor Author

arajasek commented Jul 6, 2023

Seems missing tests

fevm_events_test covers relevant cases, I think?

@Stebalien
Copy link
Member

Wait, no it's not? These are the events emitted by a single message, which is bounded by block gas limit?

Uh, yes, nvm. You're right.

@Stebalien
Copy link
Member

Okay if I keep the max limit anyway?

Only if we end up logging and dropping in that case, but even then I don't think it matters. Returning an error here will make nodes recording events halt.

@arajasek arajasek merged commit 3f71298 into master Jul 10, 2023
@arajasek arajasek deleted the asr/max-event-length branch July 10, 2023 14:37
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.

4 participants