Misc node code fixes #369
Misc node code fixes #369
Conversation
| @@ -40,15 +43,15 @@ func GetBlobMessages(in *pb.StoreChunksRequest) ([]*core.BlobMessage, error) { | |||
| } | |||
|
|
|||
| bundles := make(map[core.QuorumID]core.Bundle, len(blob.GetBundles())) | |||
| for i, chunks := range blob.GetBundles() { | |||
| quorumID := blob.GetHeader().GetQuorumHeaders()[i].QuorumId | |||
| for j, chunks := range blob.GetBundles() { | |||
There was a problem hiding this comment.
On line 41, Since GetHeader() can be nil, GetQuorumHeaders() will evaluate to invalid memory. Similarly for L44
There was a problem hiding this comment.
I added a node request validation here: #378, to make sure header is non-null
There was a problem hiding this comment.
Let's add a comment in L33 that this method assumes the inputs are valid and point to the validation method
| for i, chunks := range blob.GetBundles() { | ||
| quorumID := blob.GetHeader().GetQuorumHeaders()[i].QuorumId | ||
| for j, chunks := range blob.GetBundles() { | ||
| quorumID := blob.GetHeader().GetQuorumHeaders()[j].QuorumId | ||
| bundles[uint8(quorumID)] = make([]*encoding.Frame, len(chunks.GetChunks())) |
There was a problem hiding this comment.
casting from uint32 to uint8 is a bit dangerous, I think 256 can overwrite 0. I feel I am doing a bit too much on the audit side now
There was a problem hiding this comment.
The protobuf doesn't have uint8, that's why it's uint32 from the gRPC request, and we will cast it to uint8 inside golang world
There was a problem hiding this comment.
| @@ -40,15 +43,15 @@ func GetBlobMessages(in *pb.StoreChunksRequest) ([]*core.BlobMessage, error) { | |||
| } | |||
|
|
|||
| bundles := make(map[core.QuorumID]core.Bundle, len(blob.GetBundles())) | |||
| for i, chunks := range blob.GetBundles() { | |||
| quorumID := blob.GetHeader().GetQuorumHeaders()[i].QuorumId | |||
| for j, chunks := range blob.GetBundles() { | |||
There was a problem hiding this comment.
Let's add a comment in L33 that this method assumes the inputs are valid and point to the validation method
node/node.go
Outdated
| @@ -318,8 +318,8 @@ func (n *Node) ProcessBatch(ctx context.Context, header *core.BatchHeader, blobs | |||
| // revert all the keys for that batch. | |||
| result := <-storeChan | |||
| if result.keys != nil { | |||
| if !n.Store.DeleteKeys(ctx, result.keys) { | |||
| log.Error("Failed to delete the invalid batch that should be rolled back", "batchHeaderHash", batchHeaderHash) | |||
| if err = n.Store.DeleteKeys(ctx, result.keys); err != nil { | |||
There was a problem hiding this comment.
assign to a different err variable? The err logged in L322 should be the err in L315, but this overrides that value
| @@ -17,6 +17,8 @@ import ( | |||
| ) | |||
|
|
|||
| // Constructs a core.BatchHeader from a proto of pb.StoreChunksRequest. | |||
| // Note the StoreChunksRequest is validated as soon as it enters the node gRPC | |||
| // interface, see grpc.Server.validateStoreChunkRequest. | |||
| func GetBatchHeader(in *pb.StoreChunksRequest) (*core.BatchHeader, error) { | |||
There was a problem hiding this comment.
nit: start the comment with GetBatchHeader constructs a ...
Why are these changes needed?
Checks