Fix issues

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
parca-dev · Mar 2, 2022 · 06c4963 · 06c4963
1 parent 45c06f2
commit 06c4963
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 43 deletions.
diff --git a/pkg/buildid/buildid.go b/pkg/buildid/buildid.go
@@ -27,20 +27,20 @@ import (
 )
 
 func BuildID(path string) (string, error) {
-	exe, err := elf.Open(path)
+	f, err := elf.Open(path)
 	if err != nil {
 		return "", fmt.Errorf("failed to open elf: %w", err)
 	}
 
 	hasBuildIDSection := false
-	for _, s := range exe.Sections {
+	for _, s := range f.Sections {
 		if s.Name == ".note.go.buildid" {
 			hasBuildIDSection = true
 		}
 	}
 
 	if hasBuildIDSection {
-		exe.Close()
+		f.Close()
 
 		id, err := gobuildid.ReadFile(path)
 		if err != nil {
@@ -49,7 +49,7 @@ func BuildID(path string) (string, error) {
 
 		return hex.EncodeToString([]byte(id)), nil
 	}
-	exe.Close()
+	f.Close()
 
 	return elfBuildID(path)
 }

diff --git a/pkg/debuginfo/debuginfo.go b/pkg/debuginfo/debuginfo.go
@@ -97,7 +97,7 @@ func (di *Extractor) Upload(ctx context.Context, objFilePaths map[string]string)
 	default:
 	}
 
-	for buildID, pth := range objFilePaths {
+	for buildID, filePath := range objFilePaths {
 		exists, err := di.client.Exists(ctx, buildID)
 		if err != nil {
 			level.Error(di.logger).Log("msg", "failed to check whether build ID symbol exists", "err", err)
@@ -107,31 +107,46 @@ func (di *Extractor) Upload(ctx context.Context, objFilePaths map[string]string)
 		if !exists {
 			level.Debug(di.logger).Log("msg", "could not find symbols in server", "buildid", buildID)
 
-			hasDebugInfo, err := checkIfFileHasDebugInfo(pth)
+			hasDebugInfo, err := checkIfFileHasDebugInfo(filePath)
 			if err != nil {
-				level.Debug(di.logger).Log("msg", "failed to determine whether file has debug symbols", "file", pth, "err", err)
+				level.Debug(di.logger).Log(
+					"msg", "failed to determine whether file has debug symbols",
+					"file", filePath, "err", err,
+				)
 				continue
 			}
 
 			if !hasDebugInfo {
-				level.Debug(di.logger).Log("msg", "file does not have debug information, skipping", "file", pth, "err", err)
+				level.Debug(di.logger).Log(
+					"msg", "file does not have debug information, skipping",
+					"file", filePath, "err", err,
+				)
 				continue
 			}
 
-			debugInfoFile, err := di.extract(ctx, buildID, pth)
+			debugInfoFile, err := di.extract(ctx, buildID, filePath)
 			if err != nil {
-				level.Debug(di.logger).Log("msg", "failed to extract debug information", "buildid", buildID, "file", pth, "err", err)
+				level.Debug(di.logger).Log(
+					"msg", "failed to extract debug information",
+					"buildid", buildID, "file", filePath, "err", err,
+				)
 				continue
 			}
 
 			if err := di.uploadDebugInfo(ctx, buildID, debugInfoFile); err != nil {
 				os.Remove(debugInfoFile)
-				level.Error(di.logger).Log("msg", "failed to upload debug information", "buildid", buildID, "file", pth, "err", err)
+				level.Error(di.logger).Log(
+					"msg", "failed to upload debug information",
+					"buildid", buildID, "file", filePath, "err", err,
+				)
 				continue
 			}
 
 			os.Remove(debugInfoFile)
-			level.Info(di.logger).Log("msg", "debug information uploaded successfully", "buildid", buildID, "file", pth)
+			level.Info(di.logger).Log(
+				"msg", "debug information uploaded successfully",
+				"buildid", buildID, "file", filePath,
+			)
 			continue
 		}
 
@@ -149,10 +164,12 @@ func (di *Extractor) Extract(ctx context.Context, objFilePaths map[string]string
 	}
 
 	files := []string{}
-	for buildID, path := range objFilePaths {
-		debugInfoFile, err := di.extract(ctx, buildID, path)
+	for buildID, filePath := range objFilePaths {
+		debugInfoFile, err := di.extract(ctx, buildID, filePath)
 		if err != nil {
-			level.Error(di.logger).Log("msg", "failed to extract debug information", "buildid", buildID, "file", path, "err", err)
+			level.Error(di.logger).Log(
+				"msg", "failed to extract debug information", "buildid",
+				buildID, "file", filePath, "err", err)
 			continue
 		}
 		files = append(files, debugInfoFile)
@@ -166,7 +183,10 @@ func (di *Extractor) EnsureUploaded(ctx context.Context, objFiles []*objectfile.
 		buildID := objFile.BuildID
 		exists, err := di.client.Exists(ctx, buildID)
 		if err != nil {
-			level.Warn(di.logger).Log("msg", "failed to check whether build ID symbol exists", "err", err)
+			level.Warn(di.logger).Log(
+				"msg", "failed to check whether build ID symbol exists",
+				"buildid", buildID, "err", err,
+			)
 			continue
 		}
 
@@ -179,7 +199,10 @@ func (di *Extractor) EnsureUploaded(ctx context.Context, objFiles []*objectfile.
 				} else {
 					f, err := newDebugInfoFile(objFile)
 					if err != nil {
-						level.Debug(di.logger).Log("msg", "failed to create debug information file", "buildid", buildID, "err", err)
+						level.Debug(di.logger).Log(
+							"msg", "failed to create debug information file",
+							"buildid", buildID, "err", err,
+						)
 						continue
 					}
 					di.dbgFileCache.Add(buildID, f)
@@ -205,22 +228,34 @@ func (di *Extractor) EnsureUploaded(ctx context.Context, objFiles []*objectfile.
 
 			extractedDbgInfo, err := di.extract(ctx, buildID, objFilePath)
 			if err != nil {
-				level.Debug(di.logger).Log("msg", "failed to extract debug information", "buildid", buildID, "file", objFilePath, "err", err)
+				level.Debug(di.logger).Log(
+					"msg", "failed to extract debug information",
+					"buildid", buildID, "file", objFilePath, "err", err,
+				)
 				continue
 			}
 
 			if err := di.uploadDebugInfo(ctx, buildID, extractedDbgInfo); err != nil {
 				os.Remove(extractedDbgInfo)
-				level.Warn(di.logger).Log("msg", "failed to upload debug information", "buildid", buildID, "file", objFilePath, "err", err)
+				level.Warn(di.logger).Log(""+
+					"msg", "failed to upload debug information",
+					"buildid", buildID, "file", objFilePath, "err", err,
+				)
 				continue
 			}
 
 			os.Remove(extractedDbgInfo)
-			level.Debug(di.logger).Log("msg", "debug information uploaded successfully", "buildid", buildID, "file", objFilePath)
+			level.Debug(di.logger).Log(
+				"msg", "debug information uploaded successfully",
+				"buildid", buildID, "file", objFilePath,
+			)
 			continue
 		}
 
-		level.Debug(di.logger).Log("msg", "debug information already exist in server", "buildid", buildID)
+		level.Debug(di.logger).Log(
+			"msg", "debug information already exist in server",
+			"buildid", buildID,
+		)
 	}
 }
 
@@ -232,7 +267,9 @@ func (di *Extractor) extract(ctx context.Context, buildID, file string) (string,
 
 	hasDWARF, err := elfutils.HasDWARF(file)
 	if err != nil {
-		level.Debug(di.logger).Log("msg", "failed to determine if binary has DWARF sections", "path", file, "err", err)
+		level.Debug(di.logger).Log("msg", "failed to determine if binary has DWARF sections",
+			"path", file, "err", err,
+		)
 	}
 
 	isGo, err := elfutils.IsSymbolizableGoObjFile(file)
@@ -277,7 +314,10 @@ func (di *Extractor) extract(ctx context.Context, buildID, file string) (string,
 }
 
 func (di *Extractor) run(cmd *exec.Cmd) error {
-	level.Debug(di.logger).Log("msg", "running external binary utility command", "cmd", strings.Join(cmd.Args, " "))
+	level.Debug(di.logger).Log(
+		"msg", "running external binary utility command", "cmd",
+		strings.Join(cmd.Args, " "),
+	)
 	b := di.pool.Get().(*bytes.Buffer)
 	defer func() {
 		b.Reset()
@@ -338,8 +378,8 @@ func (di *Extractor) objcopy(ctx context.Context, file, outFile string, toRemove
 	return exec.CommandContext(ctx, "objcopy", args...)
 }
 
-func (di *Extractor) uploadDebugInfo(ctx context.Context, buildID, file string) error {
-	f, err := os.Open(file)
+func (di *Extractor) uploadDebugInfo(ctx context.Context, buildID, filePath string) error {
+	f, err := os.Open(filePath)
 	if err != nil {
 		return fmt.Errorf("failed to open temp file for debug information: %w", err)
 	}
@@ -352,7 +392,9 @@ func (di *Extractor) uploadDebugInfo(ctx context.Context, buildID, file string)
 		if _, err := di.client.Upload(ctx, buildID, f); err != nil {
 			di.logger.Log(
 				"msg", "failed to upload debug information",
-				"retry", time.Second,
+				"buildid", buildID,
+				"path", filePath,
+				"retry", expBackOff.NextBackOff(),
 				"err", err,
 			)
 		}

diff --git a/pkg/maps/maps.go b/pkg/maps/maps.go
@@ -104,7 +104,7 @@ func (c *PIDMappingFileCache) mappingForPID(pid uint32) ([]*profile.Mapping, err
 			}
 		}
 	}
-	c.cache[pid] = mapping
 
+	c.cache[pid] = mapping
 	return mapping, nil
 }
diff --git a/pkg/objectfile/cache.go b/pkg/objectfile/cache.go
@@ -14,9 +14,11 @@
 package objectfile
 
 import (
+	"errors"
 	"fmt"
 	"path"
 	"strconv"
+	"strings"
 
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
@@ -67,6 +69,13 @@ func (c *cache) ObjectFileForProcess(pid uint32, m *profile.Mapping) (*MappedObj
 
 // fromProcess opens the specified executable or library file from the process.
 func fromProcess(pid uint32, m *profile.Mapping) (*MappedObjectFile, error) {
+	if strings.EqualFold(m.File, "[vdso]") || strings.EqualFold(m.File, "[vsyscall]") {
+		return nil, errors.New("cannot load object file for mappings of [vdso] or [vsyscall]")
+	}
+	if m.File == "" {
+		return nil, errors.New("cannot load object file for mappings with empty file")
+	}
+
 	filePath := path.Join("/proc", strconv.FormatUint(uint64(pid), 10), "/root", m.File)
 	objFile, err := Open(filePath, m)
 	if err != nil {

diff --git a/pkg/objectfile/object_file.go b/pkg/objectfile/object_file.go
@@ -29,6 +29,7 @@ import (
 	"github.com/google/pprof/profile"
 
 	"github.com/parca-dev/parca-agent/internal/pprof/elfexec"
+	"github.com/parca-dev/parca-agent/pkg/buildid"
 )
 
 // Defined for testing.
@@ -63,17 +64,15 @@ func Open(filePath string, m *profile.Mapping) (*ObjectFile, error) {
 }
 
 func open(filePath string, start, limit, offset uint64, relocationSymbol string) (*ObjectFile, error) {
-	ef, err := elfOpen(filePath)
+	f, err := elfOpen(filePath)
 	if err != nil {
 		return nil, fmt.Errorf("error parsing %s: %v", filePath, err)
 	}
-	defer ef.Close()
+	defer f.Close()
 
 	buildID := ""
-	if f, err := os.Open(filePath); err == nil {
-		if id, err := elfexec.GetBuildID(f); err == nil {
-			buildID = fmt.Sprintf("%x", id)
-		}
+	if id, err := buildid.BuildID(filePath); err == nil {
+		buildID = id
 	}
 
 	var (
@@ -88,7 +87,7 @@ func open(filePath string, start, limit, offset uint64, relocationSymbol string)
 		// the name is "vmlinux" we read _stext. We can be wrong if: (1)
 		// someone passes a kernel path that doesn't contain "vmlinux" AND
 		// (2) _stext is page-aligned AND (3) _stext is not at Vaddr
-		symbols, err := ef.Symbols()
+		symbols, err := f.Symbols()
 		if err != nil && err != elf.ErrNoSymbols {
 			return nil, err
 		}
@@ -114,7 +113,7 @@ func open(filePath string, start, limit, offset uint64, relocationSymbol string)
 	// value until we have a sample address for this mapping, so that we can
 	// correctly identify the associated program segment that is needed to compute
 	// the base.
-	if _, err := elfexec.GetBase(&ef.FileHeader, elfexec.FindTextProgHeader(ef), kernelOffset, start, limit, offset); err != nil {
+	if _, err := elfexec.GetBase(&f.FileHeader, elfexec.FindTextProgHeader(f), kernelOffset, start, limit, offset); err != nil {
 		return nil, fmt.Errorf("could not identify base for %s: %v", filePath, err)
 	}
 	return &ObjectFile{

diff --git a/pkg/profiler/profiler.go b/pkg/profiler/profiler.go
@@ -114,6 +114,7 @@ func (m bpfMaps) clean() error {
 
 type CgroupProfiler struct {
 	logger log.Logger
+	reg    prometheus.Registerer
 
 	mtx    *sync.RWMutex
 	cancel func()
@@ -149,6 +150,7 @@ func NewCgroupProfiler(
 ) *CgroupProfiler {
 	return &CgroupProfiler{
 		logger:              log.With(logger, "labels", target.String()),
+		reg:                 reg,
 		mtx:                 &sync.RWMutex{},
 		target:              target,
 		profilingDuration:   profilingDuration,
@@ -196,7 +198,7 @@ func (p *CgroupProfiler) Stop() {
 	p.mtx.Lock()
 	defer p.mtx.Unlock()
 	level.Debug(p.logger).Log("msg", "stopping cgroup profiler")
-	if !prometheus.Unregister(p.missingStacks) {
+	if !p.reg.Unregister(p.missingStacks) {
 		level.Debug(p.logger).Log("msg", "cannot unregister metric")
 	}
 	if p.cancel != nil {
@@ -280,7 +282,7 @@ func (p *CgroupProfiler) Run(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("get stack traces map: %w", err)
 	}
-	p.bpfMaps = &bpfMaps{counts, stackTraces}
+	p.bpfMaps = &bpfMaps{counts: counts, stackTraces: stackTraces}
 
 	ticker := time.NewTicker(p.profilingDuration)
 	defer ticker.Stop()
@@ -444,7 +446,7 @@ func (p *CgroupProfiler) profileLoop(ctx context.Context, captureTime time.Time)
 
 					m, err := mapping.PIDAddrMapping(pid, addr)
 					if err != nil {
-						level.Debug(p.logger).Log("msg", "failed to get mapping", "err", err)
+						level.Debug(p.logger).Log("msg", "failed to get process mapping", "err", err)
 					}
 
 					var objFile *objectfile.MappedObjectFile
@@ -514,15 +516,15 @@ func (p *CgroupProfiler) profileLoop(ctx context.Context, captureTime time.Time)
 
 	// Upload debug information of the discovered object files.
 	go func() {
-		var mObjFiles []*objectfile.MappedObjectFile
+		var objFiles []*objectfile.MappedObjectFile
 		for _, mf := range mappedFiles {
-			mObjFile, err := p.objCache.ObjectFileForProcess(mf.PID, mf.Mapping)
+			objFile, err := p.objCache.ObjectFileForProcess(mf.PID, mf.Mapping)
 			if err != nil {
-				level.Debug(p.logger).Log("msg", "failed to open object file", "err", err)
+				continue
 			}
-			mObjFiles = append(mObjFiles, mObjFile)
+			objFiles = append(objFiles, objFile)
 		}
-		p.debugInfoExtractor.EnsureUploaded(ctx, mObjFiles)
+		p.debugInfoExtractor.EnsureUploaded(ctx, objFiles)
 	}()
 
 	// Resolve Kernel function names.