aionnetwork / AVM

Enabling Java code to run in a blockchain environment
https://theoan.com/
MIT License
49 stars 25 forks source link

[CLOSED] Verify fatal errors propagate to top and user code can't throw them #303

Closed aionbot closed 5 years ago

aionbot commented 5 years ago

Issue created by jeff-aion (on Friday Nov 02, 2018 at 21:03 GMT)

Note that we expect any throw of an AvmError instance to bring the AVM down, entirely. This means that we need to verify that we re-throw them from a contract termination (not just stopping in a sub-transaction) and that we bring down internal threads or document their state when this happens (since some of these cases might be things we can recover from, at the top level).

Additionally, we need to verify that it is an error for a DApp to ever attempt to throw one of these instances, themselves.

aionbot commented 5 years ago

Comment by jeff-aion (on Friday Nov 02, 2018 at 21:33 GMT)

We also need to verify that EarlyAbortException will propagate all the way up to the top-level external transaction (there may already be tests around this which we can use as the basis for this).

aionbot commented 5 years ago

Comment by jeff-aion (on Monday Nov 05, 2018 at 22:53 GMT)

We can probably generalize the TransactionTask's use of abortState into some more broadly applicable, such as "exceptions which must propagate to the top where the AVM is safe to make an executive decision".

The abort situation is one of these we expect to be very common but there are a few other examples of this, too:

A small first step which can be taken is to attempt to verify that user code is not able to throw these (any explicit references to them should probably be denied, in fact).

aionbot commented 5 years ago

Comment by jeff-aion (on Tuesday Nov 06, 2018 at 20:06 GMT)

The second point, above, is actually more general than our previous decision regarding what exceptions the user code should be able to handle. We treat as fatal (throw JvmError) all exceptions derived from java.lang.VirtualMachineError, not the more general java.lang.Error. While the more general error case probably makes sense, we minimally wanted to expose the ability to throw AssertionError, so it seems to produce less special cases to leave this as we previously decided (under #48).

That said, we still want to make sure that the user code can't throw any of these cases. While it would probably be safe (we check that it is the real exception being thrown), it wouldn't do what they are expecting (in the case of a catch) and generally seems nonsensical and something we would have to continuously demonstrate as safe (in the case of throw).

aionbot commented 5 years ago

Comment by jeff-aion (on Monday Nov 19, 2018 at 22:12 GMT)

The bulk of this work has now been merged but a few more tests, and error path implementations, are required before this can be considered complete: JvmError in any background thread should stop the entire AVM (probably need some kind of "AVM not running exception" to communicate this state).

Additionally, proving that we can handle these cases when applied to reentrant calls should also be done.