From 39a9e49233053748a252732c4f3b3fe456e4be85 Mon Sep 17 00:00:00 2001 From: cuiweixie Date: Thu, 11 Jun 2026 20:00:00 +0800 Subject: [PATCH] accounts/abi: copy selector in Pack to avoid aliasing cached method.ID ABI.Pack returned `append(method.ID, arguments...)`. method.ID is derived from crypto.Keccak256(sig)[:4], which yields a slice of length 4 but capacity 32 (the full keccak hash buffer), and it is computed once and cached on the parsed method. When a method takes no arguments, the append has nothing to copy and returns method.ID's own backing array. The caller therefore received calldata that aliased the ABI's cached selector. Mutating that calldata, or a concurrent Pack of the same no-arg method, corrupted the cached selector and broke every subsequent encode of that method. (Methods with arguments always pack to >=32 bytes, so 4+32 > 32 forces a reallocation and they were never affected.) Copy method.ID into a fresh slice before appending the arguments so the returned calldata never shares storage with the cached selector. Add a regression test that packs a no-arg method, mutates the result, and verifies both the cached selector and a subsequent pack are unaffected. --- accounts/abi/abi.go | 8 ++- accounts/abi/pack_selector_alias_test.go | 68 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 accounts/abi/pack_selector_alias_test.go diff --git a/accounts/abi/abi.go b/accounts/abi/abi.go index f75278c8b1..95bc66cce2 100644 --- a/accounts/abi/abi.go +++ b/accounts/abi/abi.go @@ -78,8 +78,12 @@ func (abi ABI) Pack(name string, args ...interface{}) ([]byte, error) { if err != nil { return nil, err } - // Pack up the method ID too if not a constructor and return - return append(method.ID, arguments...), nil + // Pack up the method ID too if not a constructor and return. The method ID + // is copied into a fresh slice instead of appended to directly: method.ID is + // derived from keccak256(sig)[:4], so it has length 4 but capacity 32, and a + // no-arg pack would otherwise return that backing array itself, aliasing the + // cached selector. + return append(append([]byte{}, method.ID...), arguments...), nil } func (abi ABI) getArguments(name string, data []byte) (Arguments, error) { diff --git a/accounts/abi/pack_selector_alias_test.go b/accounts/abi/pack_selector_alias_test.go new file mode 100644 index 0000000000..c7a5db13db --- /dev/null +++ b/accounts/abi/pack_selector_alias_test.go @@ -0,0 +1,68 @@ +// Copyright 2026 The go-ethereum Authors +// This file is part of the go-ethereum library. +// +// The go-ethereum library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The go-ethereum library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the go-ethereum library. If not, see . + +package abi + +import ( + "bytes" + "strings" + "testing" +) + +// TestPackNoArgMethodDoesNotAliasSelector is a regression test for a bug where +// ABI.Pack of a zero-argument method returned a slice that aliased the cached +// method.ID. method.ID is keccak256(sig)[:4]: length 4 but capacity 32, so +// `append(method.ID, arguments...)` with no arguments returned the selector's +// own backing array. Mutating the returned calldata then corrupted the cached +// selector for every subsequent encode of that method. +func TestPackNoArgMethodDoesNotAliasSelector(t *testing.T) { + const def = `[{ "type" : "function", "name" : "balance", "stateMutability" : "view" }]` + abi, err := JSON(strings.NewReader(def)) + if err != nil { + t.Fatal(err) + } + // selector aliases the cached method.ID; want is an independent copy of the + // expected value, so it stays correct even if the bug corrupts the cache. + selector := abi.Methods["balance"].ID + want := bytes.Clone(selector) + + data1, err := abi.Pack("balance") + if err != nil { + t.Fatal(err) + } + // The returned calldata must be exactly the 4-byte selector... + if !bytes.Equal(data1, want) { + t.Fatalf("unexpected calldata: got %x, want %x", data1, want) + } + // ...but it must not share storage with the cached selector. + if len(data1) > 0 && len(selector) > 0 && &data1[0] == &selector[0] { + t.Fatal("Pack returned a slice aliasing the cached method.ID backing array") + } + + // Mutating the first result must not affect the selector or later packs. + data1[0] ^= 0xff + + data2, err := abi.Pack("balance") + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(data2, want) { + t.Fatalf("second pack corrupted: got %x, want %x", data2, want) + } + if !bytes.Equal(abi.Methods["balance"].ID, want) { + t.Fatalf("cached method.ID corrupted: got %x, want %x", abi.Methods["balance"].ID, want) + } +}