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.

Sign up for GitHub

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

Treat go binaries without offsets as generic #1476

Merged
merged 3 commits into from
Dec 21, 2024
+19 −4
Merged

Treat go binaries without offsets as generic #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
747a021
Treat go binaries without offsets as generic
rafaelroquetto Dec 20, 2024
bb08016
Fixes panic on Go instrumentation
mariomac Dec 20, 2024
8e38155
Treat Go proxies as generic binaries
rafaelroquetto Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/internal/discover/attacher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we just need this check IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ie.InstrumentationError != nil {
ta.log.Warn("Unsupported Go program detected, using generic instrumentation", "error", ie.InstrumentationError)
} else if ie.Offsets == nil {
ta.log.Warn("Go program with null offsets detected, using generic instrumentation")
}
if ta.reusableTracer != nil {
// We need to do more than monitor PIDs. It's possible that this new
Expand Down
17 changes: 15 additions & 2 deletions pkg/internal/discover/typer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package discover

import (
"fmt"
"log/slog"
"strings"

Expand Down Expand Up @@ -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}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err == nil {
err = fmt.Errorf("identified as a Go proxy")
}

log.Debug("identified as a Go proxy")
} else {
log.Debug("identified as a generic, non-Go executable")
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rafaelroquetto rafaelroquetto Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see now, okay makes sense.

log.Warn("ELF binary appears to be a Go program, but no offsets were found",
"comm", execElf.CmdExePath, "pid", execElf.Pid)

err = fmt.Errorf("could not find any Go offsets in Go binary %s", execElf.CmdExePath)
}

log.Debug("instrumented", "comm", execElf.CmdExePath, "pid", execElf.Pid,
"child", child, "language", detectedType.String())
// Return the instrumentable without offsets, as it is identified as a generic
// (or non-instrumentable Go proxy) executable
instrumentableCache.Add(execElf.Ino, InstrumentedExecutable{Type: detectedType, Offsets: offsets, InstrumentationError: err})
return ebpf.Instrumentable{Type: detectedType, FileInfo: execElf, ChildPids: child, InstrumentationError: err}
instrumentableCache.Add(execElf.Ino, InstrumentedExecutable{Type: detectedType, Offsets: nil, InstrumentationError: err})
return ebpf.Instrumentable{Type: detectedType, Offsets: nil, FileInfo: execElf, ChildPids: child, InstrumentationError: err}
}

func (t *typer) inspectOffsets(execElf *exec.FileInfo) (*goexec.Offsets, bool, error) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/internal/goexec/structmembers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand Down
Loading