oasisprotocol / oasisprotocol/oasis-core
nit: a single unmarshallable value hides everything else
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 369
- Forks
- 151
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 6
Description
Description of the bug
nit: weird error handling.
It may well be impossible to get into an error state that triggers the error handling code below, since setStatus on a MutableTree -- which presumably at some point converts into an ImmutableTree -- marshalls an api.Status and appears to be the only place where data is marshalled, so whatever is marshalled should be unmarshallable back into an api.Status. I.e., it would be a bug in the cbor library if cbor.Unmarshal(cbor.Marshal(status), &decodedStatus) returns non-nil.
However, it seems odd that a single error will cause all other data to be inaccessible, esp when the calling code doesn't keep track of the public keys used as keys so they are no longer available. In principle, IterateRange could be used to make keys available so that the encoded keys can be converted back to signature.PublicKey values so they can be used with GetStatus.
Steps to reproduce
- Probably impossible.
Expected behavior
Generate some kind of log if there is data for which cbor.Unmarshal returns an error, since that indicates that we either (1) have memory corruption, (2) a serious bug in the cbor library, or (3) something else that we haven't thought of. In all cases, this is pretty serious and should be addressed e.g., by using ECC memory that is rowhammer proof etc, fixing the bug in the cbor library or replacing it, etc. Just passing a non-nil err up to the caller is unlikely to get things fixed, even if the cbor library puts some details in the error object. (I.e., rely on the caller to log it.) Panicking could also make sense, though maybe we want the key manager to be able to limp along until a replacement is available?
Another possibility is to have the return from GetStatuses be slices of api.Status, error tuples, rather than a slice of api.Status and a single error.
Environment
- OS:
- Versions of relevant software and tools:
Additional information
Anything else you think would be useful; including context and suggested fixes.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with go/tendermint/apps/keymanager/state.go around line 32 and trace how MutableTree, ImmutableTree, GetStatus, and GetStatuses handle cbor.Unmarshal errors. Review IterateRange as the issue suggests, then determine and document a single error-handling policy that preserves or reports other stored data.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100