Treat go binaries without offsets as generic #1476

rafaelroquetto · 2024-12-20T16:34:55Z

In the case in which we have found offsets, but these are related to a go proxy, we must set the error so that the attacher will treat this as a regular binary.

There is a small corner case in which we detect no go offsets in a target executable, but if that executable happens to have a ".gosyms" section, we will end up classifying it as a Go executable, causing issues. We attach an error in this case to have the attacher treat it as a generic executable.

Also, on ELF parse error, we return the list of missing fields, assuming that we would stop the instrumentation. There is an edge case error on which the missing fields set is empty so Beyla keeps with the instrumentation, but the returned list of fields is nil.

Closes #1472

There is a small corner case in which we detect no go offsets in a target executable, but if that executable happens to have a ".gosyms" section, we will end up classifying it as a Go executable, causing issues. We attach an error in this case to have the attacher treat it as a generic executable.

On ELF parse error, we return the list of missing fields, assuming that we would stop the instrumentation. There is an edge case error on which the missing fields set is empty so Beyla keeps with the instrumentation, but the returned list of fields is nil.

codecov · 2024-12-20T18:39:12Z

Codecov Report

Attention: Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.37%. Comparing base (1192264) to head (8e38155).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/internal/discover/typer.go	72.72%	3 Missing ⚠️
pkg/internal/discover/attacher.go	33.33%	1 Missing and 1 partial ⚠️
pkg/internal/goexec/structmembers.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1476      +/-   ##
==========================================
- Coverage   81.45%   81.37%   -0.09%     
==========================================
  Files         149      149              
  Lines       15253    15264      +11     
==========================================
- Hits        12425    12421       -4     
- Misses       2228     2240      +12     
- Partials      600      603       +3

Flag	Coverage Δ
integration-test	`59.78% <60.00%> (+<0.01%)`	⬆️
k8s-integration-test	`60.66% <13.33%> (+0.79%)`	⬆️
oats-test	`33.99% <13.33%> (-0.03%)`	⬇️
unittests	`52.05% <0.00%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

In the case in which we have found offsets, but these are related to a go proxy, we must set the error so that the attacher will treat this as a regular binary.

grcevski

The go offsets have two components, function offsets and field offsets. Where we crash right now is the field offsets being nil, while all the logic of the Go proxy only cares about the function offsets. We should leave the function offsets alone and not change anything related to the Go proxy, we should make the fix related to bad elf parsing, but also keep the checks that verify we'll avoid instrumenting as Go Executable if we found no offsets.

grcevski · 2024-12-21T13:57:01Z

pkg/internal/discover/typer.go

@@ -124,6 +125,11 @@ func (t *typer) asInstrumentable(execElf *exec.FileInfo) ebpf.Instrumentable {
 			instrumentableCache.Add(execElf.Ino, InstrumentedExecutable{Type: svc.InstrumentableGolang, Offsets: offsets})
 			return ebpf.Instrumentable{Type: svc.InstrumentableGolang, FileInfo: execElf, Offsets: offsets}
 		}
+


This is actually not enough to fix this issue. The Go proxy checks for functions missing, not offsets for fields.

grcevski · 2024-12-21T13:57:43Z

pkg/internal/discover/typer.go

@@ -146,12 +152,19 @@ func (t *typer) asInstrumentable(execElf *exec.FileInfo) ebpf.Instrumentable {

 	detectedType := exec.FindProcLanguage(execElf.Pid, execElf.ELF, execElf.CmdExePath)

+	if detectedType == svc.InstrumentableGolang && err == nil {


This should not be here, it will print a warning for our users in their logs for every go executable we discover which doesn't have any functions we care about.

This particular bit relates to this:

There is a small corner case in which we detect no go offsets in a target executable, but if that executable happens to have a ".gosyms" section, we will end up classifying it as a Go executable, causing issues. We attach an error in this case to have the attacher treat it as a generic executable.

It's a very corner case, not present in the reported issue but I discovered it while analysing the code. If offsets are null (and thus InstrumentationError is also null), but for some esoteric reason the target executable has a .gosymtab, this will cause detectedType to be InstrumentableGolang, it will trigger this codepath and crash with a null offsets pointer.

Ah I see now, okay makes sense.

grcevski · 2024-12-21T13:58:24Z

pkg/internal/goexec/structmembers.go

@@ -443,7 +443,7 @@ func structMemberOffsetsFromDwarf(data *dwarf.Data) (FieldOffsets, map[GoOffset]
 		log.Debug("inspecting fields for struct type", "type", typeName)
 		if err := readMembers(reader, structMember.fields, expectedReturns, fieldOffsets); err != nil {
 			log.Debug("error reading DWARF info", "type", typeName, "error", err)
-			return nil, expectedReturns
+			return fieldOffsets, expectedReturns


This is the only thing we need to do to fix and include a check that will avoid the null pointer in go tracers.

I believe there's more to it. This lies in the case when instrumentable.Offsets is null. The stack posted on the issue points that this call is passing a nill Offsets pointer. This pointer gets initialised here. Here we are initialising goexec.Offsets.FieldOfssets which is not directly involving in the crash (it's the toplevel goexec.Offsets being nil that is causing the crash here .

We opted to leave this in the PR because this fixes the issue you are talking about, but I reckon that happens to be a different issue.

grcevski · 2024-12-21T14:06:52Z

pkg/internal/discover/attacher.go

@@ -134,9 +134,11 @@ func (ta *TraceAttacher) getTracer(ie *ebpf.Instrumentable) bool {
 	case svc.InstrumentableGolang:
 		// gets all the possible supported tracers for a go program, and filters out
 		// those whose symbols are not present in the ELF functions list
-		if ta.Cfg.Discovery.SkipGoSpecificTracers || ta.Cfg.Discovery.SystemWide || ie.InstrumentationError != nil {
+		if ta.Cfg.Discovery.SkipGoSpecificTracers || ta.Cfg.Discovery.SystemWide || ie.InstrumentationError != nil || ie.Offsets == nil {


I think we just need this check IMO.

I think that may work indeed, but we end up being less robust by losing the distinction of the corner case (not directly related to the reported issue) in which we end up with a binary having .gosymtabs but no symbols. With the present code, we print a "prettier" message.

We still need to set the error here if you want to print a proper message when we find go proxies though.

rafaelroquetto · 2024-12-21T17:11:37Z

The go offsets have two components, function offsets and field offsets. Where we crash right now is the field offsets being nil, while all the logic of the Go proxy only cares about the function offsets. We should leave the function offsets alone and not change anything related to the Go proxy, we should make the fix related to bad elf parsing, but also keep the checks that verify we'll avoid instrumenting as Go Executable if we found no offsets.

Looking at the stack, we actually crash here on the toplevel goexec.Offsets pointer before we even reach the inner fields, as a result of the outer / toplevel Instrumentable.Offsets being null

grcevski · 2024-12-21T21:41:29Z

The go offsets have two components, function offsets and field offsets. Where we crash right now is the field offsets being nil, while all the logic of the Go proxy only cares about the function offsets. We should leave the function offsets alone and not change anything related to the Go proxy, we should make the fix related to bad elf parsing, but also keep the checks that verify we'll avoid instrumenting as Go Executable if we found no offsets.

Looking at the stack, we actually crash here on the toplevel goexec.Offsets pointer before we even reach the inner fields, as a result of the outer / toplevel Instrumentable.Offsets being null

Ah I see, I was under the impression it was only the fields that were missing.

rafaelroquetto requested review from grcevski, mariomac and marctc as code owners December 20, 2024 16:34

rafaelroquetto added the type/bug label Dec 20, 2024

rafaelroquetto mentioned this pull request Dec 20, 2024

Fixes panic on Go instrumentation #1475

Closed

rafaelroquetto and others added 2 commits December 20, 2024 12:33

Treat Go proxies as generic binaries

8e38155

In the case in which we have found offsets, but these are related to a go proxy, we must set the error so that the attacher will treat this as a regular binary.

rafaelroquetto force-pushed the fix_null_go_offsets branch from e77abdb to 8e38155 Compare December 20, 2024 18:42

grcevski reviewed Dec 21, 2024

View reviewed changes

grcevski approved these changes Dec 21, 2024

View reviewed changes

grcevski merged commit 7664338 into main Dec 21, 2024
13 checks passed

grcevski deleted the fix_null_go_offsets branch December 21, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat go binaries without offsets as generic #1476

Treat go binaries without offsets as generic #1476

rafaelroquetto commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

grcevski left a comment

grcevski Dec 21, 2024

grcevski Dec 21, 2024

rafaelroquetto Dec 21, 2024 •

edited

Loading

grcevski Dec 21, 2024

grcevski Dec 21, 2024

rafaelroquetto Dec 21, 2024

grcevski Dec 21, 2024

rafaelroquetto Dec 21, 2024

rafaelroquetto commented Dec 21, 2024

grcevski commented Dec 21, 2024

		@@ -146,12 +152,19 @@ func (t typer) asInstrumentable(execElf exec.FileInfo) ebpf.Instrumentable {

		detectedType := exec.FindProcLanguage(execElf.Pid, execElf.ELF, execElf.CmdExePath)

		if detectedType == svc.InstrumentableGolang && err == nil {

Treat go binaries without offsets as generic #1476

Treat go binaries without offsets as generic #1476

Conversation

rafaelroquetto commented Dec 20, 2024 • edited Loading

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

grcevski left a comment

Choose a reason for hiding this comment

grcevski Dec 21, 2024

Choose a reason for hiding this comment

grcevski Dec 21, 2024

Choose a reason for hiding this comment

rafaelroquetto Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

grcevski Dec 21, 2024

Choose a reason for hiding this comment

grcevski Dec 21, 2024

Choose a reason for hiding this comment

rafaelroquetto Dec 21, 2024

Choose a reason for hiding this comment

grcevski Dec 21, 2024

Choose a reason for hiding this comment

rafaelroquetto Dec 21, 2024

Choose a reason for hiding this comment

rafaelroquetto commented Dec 21, 2024

grcevski commented Dec 21, 2024

rafaelroquetto commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

rafaelroquetto Dec 21, 2024 •

edited

Loading