amass icon indicating copy to clipboard operation
amass copied to clipboard

LocalSystem.SetDataSources() always returns after 5 seconds without any indication of an error

Open UFeindschiff opened this issue 3 years ago • 0 comments

LocalSystems.SetDataSources() always returns after 5 seconds, even if the goroutines to set the datasources have not finished yet and gives no indication about doing so (like returning an error).

This prevents you from validating that data sources have actually been set for the local system as well as possibly having zombie goroutines running which may result in an attempted nil dereference and therefore a panic if LocalSystem.Shutdown() has already been called.

cfg := config.NewConfig()
sys, _ := systems.NewLocalSystem()
sys.SetDataSources(datasrcs.GetAllSources(sys)) //may return before data sources are actually set
doStuff(sys) //may use less data sources than intended
sys.Shutdown()
//a panic may happen from this point as a goroutine spawned in local.go:132 may still be running and attempting to access ressources cleared by Shutdown() as SetDataSources returns without all of them finishing

I'd propose having SetDataSources() having a variable timeout as well as return an error in such a case. Something like

func (l *LocalSystem) SetDataSources(sources []service.Service, timeout *time.Duration) error {
	f := func(src service.Service, ch chan error) { ch <- l.AddAndStart(src) }

	ch := make(chan error, len(sources))
	// Add all the data sources that successfully start to the list
	for _, src := range sources {
		go f(src, ch)
	}

       if timeout == nil {
		timeout = 30 * time.Second //I think 5 seconds is too aggressive of a default timeout  
       }

	t := time.NewTimer(timeout)
	defer t.Stop()
	for i := 0; i < len(sources); i++ {
		select {
		case <-t.C:
			return errors.New("Timeout reached. There are zombie goroutines running...")
		case <-ch:
		}
	}
        return nil
}

Alternatively, simply have SetDataSources() be blocking until all data sources have either been successfully added or failed to be added

func (l *LocalSystem) SetDataSources(sources []service.Service) {
	f := func(src service.Service, ch chan error) { ch <- l.AddAndStart(src) }

	ch := make(chan error, len(sources))
	// Add all the data sources that successfully start to the list
	for _, src := range sources {
		go f(src, ch)
	}

	for i := 0; i < len(sources); i++ {
		<-ch
	}
}

Of course, it would be preferable if all goroutines spawned by SetDataSources() and their children would be killed once the timeout is reached, but doing that wouldn't be such a trivial change as the ones proposed. Nonetheless, a non-configurable timeout of 5 seconds is too aggressive in some instances

UFeindschiff avatar Dec 06 '21 17:12 UFeindschiff