From e31d6dcf1c8ffcdafe14dd0c20020ef6ff5cf49b Mon Sep 17 00:00:00 2001 From: Ryan Hall Date: Tue, 16 Feb 2021 15:48:31 -0800 Subject: [PATCH] Fix data race in genny list Remove (#3233) --- src/dbnode/storage/id_list_gen.go | 7 +++++-- src/x/context/finalizeable_list_gen.go | 7 +++++-- src/x/generics/list/list.go | 5 ++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/dbnode/storage/id_list_gen.go b/src/dbnode/storage/id_list_gen.go index a453d9f7d2..a0b0f622af 100644 --- a/src/dbnode/storage/id_list_gen.go +++ b/src/dbnode/storage/id_list_gen.go @@ -1,4 +1,4 @@ -// Copyright (c) 2020 Uber Technologies, Inc. +// Copyright (c) 2021 Uber Technologies, Inc. // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal @@ -219,13 +219,16 @@ func (l *idList) remove(e *idElement) *idElement { // It returns the element value e.Value. // The element must not be nil. func (l *idList) Remove(e *idElement) doc.Metadata { + // read the value before returning to the pool to avoid a data race with another goroutine getting access to the + // list after it has been put back into the pool. + v := e.Value if e.list == l { // if e.list == l, l must have been initialized when e was inserted // in l or l == nil (e is a zero Element) and l.remove will crash. l.remove(e) l.Pool.put(e) } - return e.Value + return v } // PushFront inserts a new element e with value v at the front of list l and returns e. diff --git a/src/x/context/finalizeable_list_gen.go b/src/x/context/finalizeable_list_gen.go index 5f09e5dd8a..4281935fea 100644 --- a/src/x/context/finalizeable_list_gen.go +++ b/src/x/context/finalizeable_list_gen.go @@ -1,4 +1,4 @@ -// Copyright (c) 2020 Uber Technologies, Inc. +// Copyright (c) 2021 Uber Technologies, Inc. // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal @@ -218,13 +218,16 @@ func (l *finalizeableList) remove(e *finalizeableElement) *finalizeableElement { // It returns the element value e.Value. // The element must not be nil. func (l *finalizeableList) Remove(e *finalizeableElement) finalizeable { + // read the value before returning to the pool to avoid a data race with another goroutine getting access to the + // list after it has been put back into the pool. + v := e.Value if e.list == l { // if e.list == l, l must have been initialized when e was inserted // in l or l == nil (e is a zero Element) and l.remove will crash. l.remove(e) l.Pool.put(e) } - return e.Value + return v } // PushFront inserts a new element e with value v at the front of list l and returns e. diff --git a/src/x/generics/list/list.go b/src/x/generics/list/list.go index 56661c8e15..82b9653d42 100644 --- a/src/x/generics/list/list.go +++ b/src/x/generics/list/list.go @@ -199,13 +199,16 @@ func (l *List) remove(e *Element) *Element { // It returns the element value e.Value. // The element must not be nil. func (l *List) Remove(e *Element) ValueType { + // read the value before returning to the pool to avoid a data race with another goroutine getting access to the + // list after it has been put back into the pool. + v := e.Value if e.list == l { // if e.list == l, l must have been initialized when e was inserted // in l or l == nil (e is a zero Element) and l.remove will crash. l.remove(e) l.Pool.put(e) } - return e.Value + return v } // PushFront inserts a new element e with value v at the front of list l and returns e.