Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
yanhua1133 opened this issue Jul 24, 2024 · 1 comment · May be fixed by #7216
Open

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

yanhua1133 opened this issue Jul 24, 2024 · 1 comment · May be fixed by #7216
Labels
bug Something isn't working community Community contribution docker Docker query terraform Terraform query

Comments

@yanhua1133
Copy link

yanhua1133 commented Jul 24, 2024

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).

  1. 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 			}
@yanhua1133 yanhua1133 added bug Something isn't working community Community contribution labels Jul 24, 2024
@github-actions github-actions bot added docker Docker query terraform Terraform query labels Jul 24, 2024
yanhua1133 pushed a commit to yanhua1133/kics that referenced this issue Jul 24, 2024
@cx-monicac
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working community Community contribution docker Docker query terraform Terraform query
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants