Data race in ProcessTree

[subkin13]

Could be a write and read to pt.procfsOnce.
Solution: move if pt.procfsOnce == nil { before if pt.procfsChan == nil {

func (pt *ProcessTree) feedFromProcFSLoop() {
	go func() {
		for {
			select {
			case <-time.After(15 * time.Second):
				pt.procfsOnce = new(sync.Once) // reset the once every 15 seconds
			case <-pt.ctx.Done():
				return
			case givenPid := <-pt.procfsChan:
				err := pt.FeedFromProcFS(givenPid)
				if err != nil && debugMsgs {
					logger.Debugw("proctree from procfs (loop)", "err", err)
				}
			}
		}
	}()
}

// FeedFromProcFSAsync feeds the process tree with data from procfs asynchronously.
func (pt *ProcessTree) FeedFromProcFSAsync(givenPid int) {
	if pt.procfsChan == nil {
		logger.Debugw("starting procfs proctree loop") // will tell if called more than once
		pt.procfsChan = make(chan int, 1000)
		pt.feedFromProcFSLoop()
	}
	if pt.procfsOnce == nil {
		pt.procfsOnce = new(sync.Once)
	}

[geyslan]

Could be a write and read to pt.procfsOnce.

Thanks @subkin13. It really seems to be the case.

Perhaps a mutex control only for the procfsOnce? @NDStrahilevitz WDYT?

[NDStrahilevitz]

@geyslan Not so familiar with this code i'm afraid. @AlonZivony Would be more relevant here.
Anyway, from a glance at this, it seems like this procfsOnce should be initialized on construction, then no nil check is required. If there is an additional condition to trigger procfsOnce, it should be explicit and perhaps concurrency controlled if needed.
Ditto for the channel, not sure why it is lazily initialized, and not initialized at construction.

[AlonZivony]

@geyslan can you elaborate on the issue here?

[geyslan]

@geyslan can you elaborate on the issue here?

The goroutine triggered by feedFromProcFSLoop() might timeout, right? If so it could be writing to pt.procfsOnce while it's read in?

tracee/pkg/proctree/proctree_procfs.go

Lines 51 to 59 in 39fc6cc

	// feed the loop without blocking (if the loop is busy, given pid won't be processed)
	select {
	case pt.procfsChan <- givenPid: // feed the loop
	default:
	pt.procfsOnce.Do(func() {
	// only log once if the loop is busy (avoid spamming the logs), once is reset every 15s
	logger.Debugw("procfs proctree loop is busy")
	})
	}

If so, what @NDStrahilevitz says about lazy initialization makes sense and I put on top of that we should not overwrite the Once - if not possible, we should change the mechanism dodging surprises.

[AlonZivony]

But does it cause an error?
If it does, it would be nice to put it here.

[geyslan]

But does it cause an error? If it does, it would be nice to put it here.

Agree. @subkin13 could you provide an output showing the error? Please include steps to reproduce.

[subkin13]

I just ran it with -race and it found it

[AlonZivony]

Hmm so I guess we would like to solve it just so it won't be considered a "race", but I don't think we really have an issue here.
In the end the only purpose here is to reduce debug spam, so its not an issue if there is a race here.
I don't know the go's race checker very good, but maybe we can envelope the pointer with atomic.Pointer? Just an idea.

[subkin13]

how about initializing procfsOnce in NewProcessTree?

procTree := &ProcessTree{
		processes:  processes,
		threads:    threads,
		**procfsOnce: new(sync.Once),**
		ctx:        ctx,
		mutex:      &sync.RWMutex{},
	}

[NDStrahilevitz]

how about initializing procfsOnce in NewProcessTree?

procTree := &ProcessTree{
		processes:  processes,
		threads:    threads,
		**procfsOnce: new(sync.Once),**
		ctx:        ctx,
		mutex:      &sync.RWMutex{},
	}

That's the most sensible solution imo. Ditto for the channel.

[AlonZivony]

how about initializing procfsOnce in NewProcessTree?

procTree := &ProcessTree{
		processes:  processes,
		threads:    threads,
		**procfsOnce: new(sync.Once),**
		ctx:        ctx,
		mutex:      &sync.RWMutex{},
	}

That's the most sensible solution imo. Ditto for the channel.

LGTM :)