bug(terraform): a deadlock in filesystem.go since v1.7.13

[yanhua1133]

A deadlock in filesystem.go since v1.7.13

Expected Behavior

No hang

Actual Behavior

Hang

Steps to Reproduce the Problem

git clone https://github.com/Checkmarx/kics.git
cd kics/test/fixtures
docker run -t -v ./test_terra_cache/:/path -w /path checkmarx/kics:v2.1.1-alpine scan -p /path -o "/path/"

Analysis

Please see below the code snippet from filesystem.go.

1. In line 235 RLock is applied
2. Next in line 243, AddExcluded is called, so the control flow goes to line 62
3. Next in line 76, Lock is applied without releasing the RLock in line 235, causing deadlock

61  // AddExcluded add new excluded files to the File System Source Provider
62  func (s *FileSystemSourceProvider) AddExcluded(excludePaths []string) error {
63  	for _, excludePath := range excludePaths {
64  		info, err := os.Stat(excludePath)
65  		if err != nil {
66  			if os.IsNotExist(err) {
67  				continue
68  			}
69  			if sysErr, ok := err.(*ioFs.PathError); ok {
70  				log.Warn().Msgf("Failed getting file info for file '%s', Skipping due to: %s, Error number: %d",
71  					excludePath, sysErr, sysErr.Err.(syscall.Errno))
72  				continue
73  			}
74  			return errors.Wrap(err, "failed to open excluded file")
75  		}
76  		s.mu.Lock()
77  		if _, ok := s.excludes[info.Name()]; !ok {
78  			s.excludes[info.Name()] = make([]os.FileInfo, 0)
79  		}
80  		s.excludes[info.Name()] = append(s.excludes[info.Name()], info)
81  		s.mu.Unlock()
82  	}
83  	return nil
84  }

233 func (s *FileSystemSourceProvider) checkConditions(info os.FileInfo, extensions model.Extensions,
234 	path string, resolved bool) (bool, error) {
235 	s.mu.RLock()
236 	defer s.mu.RUnlock()
237
238 	if info.IsDir() {
239 		// exclude terraform cache folders
240 		if queryRegexExcludeTerraCache.MatchString(path) {
241 			log.Info().Msgf("Directory ignored: %s", path)
242
243 			err := s.AddExcluded([]string{info.Name()})
244 			if err != nil {
245 				return true, err
246 			}
247 			return true, filepath.SkipDir
248 		}
...
257 		return false, nil
258 	}
...
270 }

The bug "luckily" escaped from the test and many scenarios, only because the program logic is problematic:
In line 64, os.Stat(excludePath) often returns an err because often cases the working directory does not contain a sub directory named excludePath (which is a relative path), thereby returns early/continue (in lines 67, 72 & 74) before it hits the deadlock code in line 76.

To enforce it to happen, just make excludePath a sub directory of the working directory by:

docker run -t -v ./test_terra_cache/:/path -w /path checkmarx/kics:v2.1.1-alpine scan -p /path -o "/path/"

Alternatively, use the kics command-line in the same directory as the scan target specified by --path.

Possible fix:

1. Just remove the call of AddExcluded

-- 243 			err := s.AddExcluded([]string{info.Name()})
-- 244 			if err != nil {
-- 245 				return true, err
-- 246 			}

as in line 247 filepath.SkipDir is returned anyway to skip the dir (i.e., .terraform).

2. Release the lock before locking again

++                      s.mu.RUnlock()
243 			err := s.AddExcluded([]string{info.Name()})
++                      s.mu.RLock()
244 			if err != nil {
245 				return true, err
246 			}

[cx-monicac]

Hi @yanhua1133

Thank you for bringing this bug to our attention we opened an internal bug and will work on it as soon as possible.