github-vet / github-vet/rangeloop-pointer-findings

containers/buildah: add.go; 120 LoC

Open
#18,146 0 comments 0 reactions 0 assignees View on GitHub
fresh large
Dominant language
No language data
Stars
0
Forks
0
PR merge metrics
PR metrics pending

Description

Found a possible issue in [containers/buildah](https://www.github.com/containers/buildah) at [add.go](https://github.com/containers/buildah/blob/02b914b1c424c151d0a32192876a37daf5927981/add.go#L396-L515)

Below is the message reported by the analyzer for this snippet of code. Beware that the analyzer only reports the first issue it finds, so please do not limit your consideration to the contents of the below message.

> range-loop variable glob used in defer or goroutine at line 456

[Click here to see the code in its original context.](https://github.com/containers/buildah/blob/02b914b1c424c151d0a32192876a37daf5927981/add.go#L396-L515)

Click here to show the 120 line(s) of Go which triggered the analyzer.

```go
for _, glob := range localSourceStat.Globbed {
rel, err := filepath.Rel(contextDir, glob)
if err != nil {
return errors.Wrapf(err, "error computing path of %q relative to %q", glob, contextDir)
}
if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
return errors.Errorf("possible escaping context directory error: %q is outside of %q", glob, contextDir)
}
// Check for dockerignore-style exclusion of this item.
if rel != "." {
excluded, err := pm.Matches(filepath.ToSlash(rel)) // nolint:staticcheck
if err != nil {
return errors.Wrapf(err, "error checking if %q(%q) is excluded", glob, rel)
}
if excluded {
// non-directories that are excluded are excluded, no question, but
// directories can only be skipped if we don't have to allow for the
// possibility of finding things to include under them
globInfo := localSourceStat.Results[glob]
if !globInfo.IsDir || !includeDirectoryAnyway(rel, pm) {
continue
}
}
} else {
// Make sure we don't trigger a "copied nothing" error for an empty context
// directory if we were told to copy the context directory itself. We won't
// actually copy it, but we need to make sure that we don't produce an error
// due to potentially not having anything in the tarstream that we passed.
itemsCopied++
}
st := localSourceStat.Results[glob]
pipeReader, pipeWriter := io.Pipe()
wg.Add(1)
go func() {
renamedItems := 0
writer := io.WriteCloser(pipeWriter)
if renameTarget != "" {
writer = newTarFilterer(writer, func(hdr *tar.Header) (bool, bool, io.Reader) {
hdr.Name = renameTarget
renamedItems++
return false, false, nil
})
}
writer = newTarFilterer(writer, func(hdr *tar.Header) (bool, bool, io.Reader) {
itemsCopied++
return false, false, nil
})
getOptions := copier.GetOptions{
UIDMap: srcUIDMap,
GIDMap: srcGIDMap,
Excludes: options.Excludes,
ExpandArchives: extract,
ChownDirs: chownDirs,
ChmodDirs: nil,
ChownFiles: chownFiles,
ChmodFiles: nil,
StripSetuidBit: options.StripSetuidBit,
StripSetgidBit: options.StripSetgidBit,
StripStickyBit: options.StripStickyBit,
}
getErr = copier.Get(contextDir, contextDir, getOptions, []string{glob}, writer)
closeErr = writer.Close()
if renameTarget != "" && renamedItems > 1 {
renameErr = errors.Errorf("internal error: renamed %d items when we expected to only rename 1", renamedItems)
}
wg.Done()
}()
wg.Add(1)
go func() {
if st.IsDir {
b.ContentDigester.Start("dir")
} else {
b.ContentDigester.Start("file")
}
hashCloser := b.ContentDigester.Hash()
hasher := io.Writer(hashCloser)
if options.Hasher != nil {
hasher = io.MultiWriter(hasher, options.Hasher)
}
if options.DryRun {
_, putErr = io.Copy(hasher, pipeReader)
} else {
putOptions := copier.PutOptions{
UIDMap: destUIDMap,
GIDMap: destGIDMap,
DefaultDirOwner: chownDirs,
DefaultDirMode: nil,
ChownDirs: nil,
ChmodDirs: nil,
ChownFiles: nil,
ChmodFiles: nil,
IgnoreDevices: rsystem.RunningInUserNS(),
}
putErr = copier.Put(mountPoint, extractDirectory, putOptions, io.TeeReader(pipeReader, hasher))
}
hashCloser.Close()
pipeReader.Close()
wg.Done()
}()
wg.Wait()
if getErr != nil {
getErr = errors.Wrapf(getErr, "error reading %q", src)
}
if closeErr != nil {
closeErr = errors.Wrapf(closeErr, "error closing %q", src)
}
if renameErr != nil {
renameErr = errors.Wrapf(renameErr, "error renaming %q", src)
}
if putErr != nil {
putErr = errors.Wrapf(putErr, "error storing %q", src)
}
multiErr = multierror.Append(getErr, closeErr, renameErr, putErr)
if multiErr != nil && multiErr.ErrorOrNil() != nil {
if len(multiErr.Errors) > 1 {
return multiErr.ErrorOrNil()
}
return multiErr.Errors[0]
}
}

```

Leave a reaction on this issue to contribute to the project by classifying this instance as a **Bug** :-1:, **Mitigated** :+1:, or **Desirable Behavior** :rocket:
See the descriptions of the classifications [here](https://github.com/github-vet/rangeclosure-findings#how-can-i-help) for more information.

commit ID: 02b914b1c424c151d0a32192876a37daf5927981

Contributor guide

No contributing guide indexed for this repository

Research direction

Inspect containers/buildah/add.go at lines 396-515, especially the goroutines inside the loop over localSourceStat.Globbed. Determine whether the reported range-loop capture affects behavior in this context, then classify the finding as Bug, Mitigated, or Desirable Behavior using the issue's reaction options.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
devops
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.