m/node: fix appending to read-only slices
`append` modifies its first argument if there is sufficient capacity
instead of allocating a new slice. If the slice to be appended to is
supposed to be read-only, this can lead to unexpected aliasing.
An example of what can go wrong, here with the consensus client:
l := NewLocal(nil)
sub1, _ := l.Sub("I")
sub2, _ := sub1.Sub("am")
sub3, _ := sub2.Sub("a")
dog, _ := sub3.Sub("dog")
_, _ = sub3.Sub("cat")
fmt.Print(dog.(*local).path)
Result before this change: "I am a cat"
Result after this change: "I am a dog"
After creating a subnamespace of length 3, the capacity of the `path` is
4, so any subnamespace will share the same slice. The fix is to always
ensure a new slice is allocated.
Impact
------
For the consensus client, Sub is currently never called multiple times
on the same namespace, so there is no impact there. In case of the dhcp
client and rpc resolver, the slices that are appended to are slice
literals in all cases, which don't have extra capacity.
But for the curator `etcdPrefix`, `p.parts` has capacity 1 larger than
the length, due to the slicing in `newEtcdPrefix`. That means that
concurrent calls to `Key` can overwrite each other's `path`. It looks
like `Key` can in fact be called concurrently, which means there is
potential for data corruption to occur before this change.
Change-Id: I28e7dc797365c2beea97023ed31a20eea599e678
Reviewed-on: https://review.monogon.dev/c/monogon/+/2873
Vouch-Run-CI: Lorenz Brun <lorenz@monogon.tech>
Tested-by: Jenkins CI
Reviewed-by: Serge Bazanski <serge@monogon.tech>
diff --git a/metropolis/node/core/consensus/client/client.go b/metropolis/node/core/consensus/client/client.go
index 821919b..70e27aa 100644
--- a/metropolis/node/core/consensus/client/client.go
+++ b/metropolis/node/core/consensus/client/client.go
@@ -111,7 +111,7 @@
}
sub := &local{
root: l.root,
- path: append(l.path, space),
+ path: append(append([]string(nil), l.path...), space),
}
sub.populate()
return sub, nil
diff --git a/metropolis/node/core/curator/state.go b/metropolis/node/core/curator/state.go
index c1969d4..39837a7 100644
--- a/metropolis/node/core/curator/state.go
+++ b/metropolis/node/core/curator/state.go
@@ -73,7 +73,7 @@
if strings.Contains(id, "/") {
return "", fmt.Errorf("invalid id: cannot contain / character")
}
- path := append(p.parts, id)
+ path := append(append([]string(nil), p.parts...), id)
return "/" + strings.Join(path, "/"), nil
}
diff --git a/metropolis/node/core/network/dhcp4c/dhcpc.go b/metropolis/node/core/network/dhcp4c/dhcpc.go
index 9bb70dd..bfb2676 100644
--- a/metropolis/node/core/network/dhcp4c/dhcpc.go
+++ b/metropolis/node/core/network/dhcp4c/dhcpc.go
@@ -209,9 +209,10 @@
// managed through external means like a routing protocol, setting the default
// route is also required. A simple example with the callback package thus
// looks like this:
-// c := dhcp4c.NewClient(yourInterface)
-// c.LeaseCallback = callback.Compose(callback.ManageIP(yourInterface), callback.ManageDefaultRoute(yourInterface))
-// c.Run(ctx)
+//
+// c := dhcp4c.NewClient(yourInterface)
+// c.LeaseCallback = callback.Compose(callback.ManageIP(yourInterface), callback.ManageDefaultRoute(yourInterface))
+// c.Run(ctx)
func NewClient(iface *net.Interface) (*Client, error) {
broadcastConn := transport.NewBroadcastTransport(iface)
@@ -350,7 +351,7 @@
opts.Update(dhcpv4.OptClientIdentifier(c.ClientIdentifier))
}
if t == dhcpv4.MessageTypeDiscover || t == dhcpv4.MessageTypeRequest || t == dhcpv4.MessageTypeInform {
- opts.Update(dhcpv4.OptParameterRequestList(append(c.RequestedOptions, internalOptions...)...))
+ opts.Update(dhcpv4.OptParameterRequestList(append(append(dhcpv4.OptionCodeList(nil), c.RequestedOptions...), internalOptions...)...))
opts.Update(dhcpv4.OptMaxMessageSize(c.maxMsgSize()))
if c.VendorClassIdentifier != "" {
opts.Update(dhcpv4.OptClassIdentifier(c.VendorClassIdentifier))
diff --git a/metropolis/node/core/rpc/resolver/resolver.go b/metropolis/node/core/rpc/resolver/resolver.go
index 5b89f73..2e1fc99 100644
--- a/metropolis/node/core/rpc/resolver/resolver.go
+++ b/metropolis/node/core/rpc/resolver/resolver.go
@@ -13,10 +13,9 @@
"google.golang.org/grpc"
"google.golang.org/grpc/keepalive"
- "source.monogon.dev/metropolis/node/core/curator/watcher"
-
common "source.monogon.dev/metropolis/node"
apb "source.monogon.dev/metropolis/node/core/curator/proto/api"
+ "source.monogon.dev/metropolis/node/core/curator/watcher"
cpb "source.monogon.dev/metropolis/proto/common"
)
@@ -213,6 +212,7 @@
// Use a keepalive to make sure we time out fast if the node we're connecting to
// partitions.
+ opts = append([]grpc.DialOption(nil), opts...)
opts = append(opts, grpc.WithResolvers(r), grpc.WithKeepaliveParams(keepalive.ClientParameters{
Time: 10 * time.Second,
Timeout: 5 * time.Second,
@@ -331,6 +331,7 @@
// partitions. This is particularly critical for the leader updater, as we want
// to know as early as possible that this happened, so that we can move over to
// another node.
+ opts = append([]grpc.DialOption(nil), opts...)
opts = append(opts, grpc.WithKeepaliveParams(keepalive.ClientParameters{
Time: 10 * time.Second,
Timeout: 5 * time.Second,