fixup! fix!: keep deserialised state stable until explicit mutation

coding.go

@@ -71,10 +71,14 @@ func fromImmutableNode(encoded *immutableProtoNode) *ProtoNode {
 		link.Cid = c
 		n.links[i] = link
 	}
+	// we don't set n.linksDirty because the order of the links list from
+	// serialized form needs to be stable, until we start mutating the ProtoNode
 	return n
 }
 func (n *ProtoNode) marshalImmutable() (*immutableProtoNode, error) {
-	// ensure links are sorted, but don't modify existing link slice
+	// ensure links are sorted, but don't modify existing link slice; even if
+	// linksDirty is true, the links list may not be sorted if it came from a
+	// badly serialized form with unsorted links
 	links := n.Links()
 	sort.Stable(LinkSlice(links))
 	nd, err := qp.BuildMap(dagpb.Type.PBNode, 2, func(ma ipld.MapAssembler) {
@@ -136,8 +140,9 @@ func (n *ProtoNode) GetPBNode() *pb.PBNode {
 		}
 	}
 
-	// Ensure links are sorted prior to encode. They may not have come sorted if
-	// we deserialized a badly encoded form that didn't have links already sorted.
+	// Ensure links are sorted prior to encode, regardless of `linksDirty`. They
+	// may not have come sorted if we deserialized a badly encoded form that
+	// didn't have links already sorted.
 	sort.Stable(pbLinkSlice(pbn.Links))
 
 	if len(n.data) > 0 {

node.go

@@ -42,8 +42,9 @@ type immutableProtoNode struct {
 // this mutable protonode implementation is needed to support go-unixfs,
 // the only library that implements both read and write for UnixFS v1.
 type ProtoNode struct {
-	links []*format.Link
-	data  []byte
+	links      []*format.Link
+	linksDirty bool
+	data       []byte
 
 	// cache encoded/marshaled value
 	encoded *immutableProtoNode
@@ -154,13 +155,13 @@ func (n *ProtoNode) AddRawLink(name string, l *format.Link) error {
 		Size: l.Size,
 		Cid:  l.Cid,
 	})
-	sort.Stable(LinkSlice(n.links))
+	n.linksDirty = true // needs a sort
 	return nil
 }
 
 // RemoveNodeLink removes a link on this node by the given name. If there are
 // no links with this name, ErrLinkNotFound will be returned. If there are more
-// thank one link with this name, they will all be removed.
+// than one link with this name, they will all be removed.
 func (n *ProtoNode) RemoveNodeLink(name string) error {
 	n.encoded = nil
 
@@ -180,6 +181,10 @@ func (n *ProtoNode) RemoveNodeLink(name string) error {
 	}
 
 	n.links = ref
+	// Even though a removal won't change sorting, this node may have come from
+	// a deserialized state with badly sorted links. Now that we are mutating,
+	// we need to ensure the resulting link list is sorted when it gets consumed.
+	n.linksDirty = true
 
 	return nil
 }
@@ -223,8 +228,10 @@ func (n *ProtoNode) GetLinkedNode(ctx context.Context, ds format.DAGService, nam
 	return lnk.GetNode(ctx, ds)
 }
 
-// Copy returns a copy of the node.
-// NOTE: Does not make copies of Node objects in the links.
+// Copy returns a copy of the node. The resulting node will have a properly
+// sorted Links list regardless of whether the original came from a badly
+// serialized form that didn't have a sorted list.
+// NOTE: This does not make copies of Node objects in the links.
 func (n *ProtoNode) Copy() format.Node {
 	nnode := new(ProtoNode)
 	if len(n.data) > 0 {
@@ -235,6 +242,10 @@ func (n *ProtoNode) Copy() format.Node {
 	if len(n.links) > 0 {
 		nnode.links = make([]*format.Link, len(n.links))
 		copy(nnode.links, n.links)
+		// Sort links regardless of linksDirty state, this may have come from a
+		// serialized form that had badly sorted links, in which case linksDirty
+		// will not be true.
+		sort.Stable(LinkSlice(nnode.links))
 	}
 
 	nnode.builder = n.builder
@@ -333,15 +344,21 @@ func (n *ProtoNode) UnmarshalJSON(b []byte) error {
 	}
 
 	n.data = s.Data
-	// links may not be sorted after deserialization, but we don't change
+	// Links may not be sorted after deserialization, but we don't change
 	// them until we mutate this node since we're representing the current,
-	// as-serialized state
+	// as-serialized state. So n.linksDirty is not set here.
 	n.links = s.Links
 	return nil
 }
 
 // MarshalJSON returns a JSON representation of the node.
 func (n *ProtoNode) MarshalJSON() ([]byte, error) {
+	if n.linksDirty {
+		// there was a mutation involving links, make sure we sort
+		sort.Stable(LinkSlice(n.links))
+		n.linksDirty = false
+	}
+
 	out := map[string]interface{}{
 		"data":  n.data,
 		"links": n.links,
@@ -387,6 +404,11 @@ func (n *ProtoNode) Multihash() mh.Multihash {
 
 // Links returns a copy of the node's links.
 func (n *ProtoNode) Links() []*format.Link {
+	if n.linksDirty {
+		// there was a mutation involving links, make sure we sort
+		sort.Stable(LinkSlice(n.links))
+		n.linksDirty = false
+	}
 	links := make([]*format.Link, len(n.links))
 	copy(links, n.links)
 	return links
@@ -397,7 +419,7 @@ func (n *ProtoNode) Links() []*format.Link {
 func (n *ProtoNode) SetLinks(links []*format.Link) {
 	n.links = make([]*format.Link, len(links))
 	copy(n.links, links)
-	sort.Stable(LinkSlice(n.links))
+	n.linksDirty = true // needs a sort
 }
 
 // Resolve is an alias for ResolveLink.
@@ -429,6 +451,12 @@ func (n *ProtoNode) Tree(p string, depth int) []string {
 		return nil
 	}
 
+	if n.linksDirty {
+		// there was a mutation involving links, make sure we sort
+		sort.Stable(LinkSlice(n.links))
+		n.linksDirty = false
+	}
+
 	out := make([]string, 0, len(n.links))
 	for _, lnk := range n.links {
 		out = append(out, lnk.Name)