Merge 5dbf26db90 into 12eabbd76d

eth/protocols/snap/handler.go

@@ -52,6 +52,16 @@ const (
 	// If we spend too much time, then it's a fairly high chance of timing out
 	// at the remote side, which means all the work is in vain.
 	maxTrieNodeTimeSpent = 5 * time.Second
+
+	// maxTrieNodePathLength is the longest a compact-encoded trie path can be
+	// while still addressing a node in any Ethereum state trie. Trie keys are
+	// Keccak256 hashes (32 bytes = 64 nibbles). The theoretical compact-encoded
+	// upper bound is 33 bytes (64 nibbles + 1 prefix byte), but in an MPT the
+	// value node is always embedded in its parent and is never addressed as a
+	// standalone node, so the hexary path length is at most 63 nibbles and the
+	// compact encoding never exceeds 32 bytes. Anything longer is structurally
+	// invalid and the handler skips it without performing a trie traversal.
+	maxTrieNodePathLength = 32
 )
 
 // Handler is a callback to invoke from an outside runner after the boilerplate

eth/protocols/snap/handler_test.go

@@ -220,6 +220,73 @@ func TestServiceGetAccessListsQueryByteLimit(t *testing.T) {
 	}
 }
 
+// TestServiceGetTrieNodesQueryPathLength verifies that ServiceGetTrieNodesQuery
+// rejects structurally invalid (over-length) compact-encoded paths without
+// allocating in compactToHex or walking the trie. Trie keys are Keccak256
+// hashes; value nodes are always embedded in their parent and are never
+// addressed as standalone nodes, so the longest compact-encoded path that can
+// match a node is 32 bytes. Anything longer cannot match a node in any state
+// trie. A peer sending such a path (up to the 10MB request size cap) would
+// otherwise force the server to allocate 2*len(path)+1 nibbles per entry and
+// start a doomed traversal.
+func TestServiceGetTrieNodesQueryPathLength(t *testing.T) {
+	t.Parallel()
+
+	bc, _, _ := getChainWithBALs(1, 100)
+	defer bc.Stop()
+
+	root := bc.CurrentBlock().Root
+
+	// One byte beyond the spec-aligned cap (33 bytes).
+	overlong := make([]byte, maxTrieNodePathLength+1)
+	for i := range overlong {
+		overlong[i] = byte(i)
+	}
+	// Far over the cap, exercising the worst-case allocation path.
+	gigantic := make([]byte, 1024)
+	for i := range gigantic {
+		gigantic[i] = byte(i)
+	}
+
+	tests := []struct {
+		name  string
+		paths []TrieNodePathSet
+	}{
+		{
+			name:  "account path one byte over limit",
+			paths: []TrieNodePathSet{{overlong}},
+		},
+		{
+			name:  "account path far over limit",
+			paths: []TrieNodePathSet{{gigantic}},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			encPaths, err := rlp.EncodeToRawList(tt.paths)
+			if err != nil {
+				t.Fatalf("encode paths: %v", err)
+			}
+			req := &GetTrieNodesPacket{
+				ID:    1,
+				Root:  root,
+				Paths: encPaths,
+				Bytes: softResponseLimit,
+			}
+			nodes, err := ServiceGetTrieNodesQuery(bc, req)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if len(nodes) != 1 {
+				t.Fatalf("expected 1 placeholder node, got %d", len(nodes))
+			}
+			if len(nodes[0]) != 0 {
+				t.Errorf("expected nil placeholder for oversized path, got %x", nodes[0])
+			}
+		})
+	}
+}
+
 // TestGetAccessListResponseDecoding verifies that an AccessListsPacket
 // round-trips through RLP encode/decode, preserving positional
 // correspondence and correctly representing absent BALs as empty strings.

eth/protocols/snap/handlers.go

@@ -469,13 +469,22 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) (
 			if accKey == nil {
 				return nodes, fmt.Errorf("%w: invalid account node request", errBadRequest)
 			}
-			blob, resolved, err := accTrie.GetNode(accKey)
+			// Skip structurally invalid paths (longer than maxTrieNodePathLength)
-			loads += resolved // always account database reads, even for failures
+			// without traversing the trie. compactToHex would otherwise allocate
-			if err != nil {
+			// 2*len(accKey)+1 bytes for a key that cannot possibly match any node.
-				break
+			// A nil placeholder is appended to preserve positional alignment with
+			// the request, matching the existing behavior for non-existent nodes.
+			if len(accKey) > maxTrieNodePathLength {
+				nodes = append(nodes, nil)
+			} else {
+				blob, resolved, err := accTrie.GetNode(accKey)
+				loads += resolved // always account database reads, even for failures
+				if err != nil {
+					break
+				}
+				nodes = append(nodes, blob)
+				bytes += uint64(len(blob))
 			}
-			nodes = append(nodes, blob)
-			bytes += uint64(len(blob))
 
 		default:
 			// Storage slots requested, open the storage trie and retrieve from there
@@ -513,13 +522,21 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) (
 				if err != nil {
 					return nil, fmt.Errorf("%w: invalid storage key: %v", errBadRequest, err)
 				}
-				blob, resolved, err := stTrie.GetNode(path)
+				// Skip structurally invalid paths (longer than maxTrieNodePathLength)
-				loads += resolved // always account database reads, even for failures
+				// without traversing the storage trie. See the account-path branch
-				if err != nil {
+				// above for the rationale; we still append a nil placeholder so
-					break
+				// the response stays positionally aligned with the request.
+				if len(path) > maxTrieNodePathLength {
+					nodes = append(nodes, nil)
+				} else {
+					blob, resolved, err := stTrie.GetNode(path)
+					loads += resolved // always account database reads, even for failures
+					if err != nil {
+						break
+					}
+					nodes = append(nodes, blob)
+					bytes += uint64(len(blob))
 				}
-				nodes = append(nodes, blob)
-				bytes += uint64(len(blob))
 
 				// Sanity check limits to avoid DoS on the store trie loads
 				if bytes > req.Bytes || loads > maxTrieNodeLookups || time.Since(start) > maxTrieNodeTimeSpent {