oasisprotocol / oasisprotocol/oasis-core

nit: a single unmarshallable value hides everything else

Open
#2,172 0 comments 0 reactions 0 assignees View on GitHub

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.

https://github.com/oasislabs/ekiden/blob/4e9f4a306ea0e76a3daf459e040dd517b801e985/go/tendermint/apps/keymanager/state.go#L32

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

  1. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.