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

fix: move saving logic outside captor package #25

Merged
merged 4 commits into from
Aug 4, 2024
Merged

fix: move saving logic outside captor package #25

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e92ecd
fix: move saving logic outside captore package
alegrey91 Aug 3, 2024
180a6b2
fix: add missing return in build command
alegrey91 Aug 3, 2024
e4d685f
remove os.Exit in cmd/capture.go
alegrey91 Aug 4, 2024
a305bae
wrap errors with %w instead of using %v
alegrey91 Aug 4, 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
1 change: 1 addition & 0 deletions cmd/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ var buildCmd = &cobra.Command{

if err := profileFile.Chmod(0644); err != nil {
fmt.Printf("error setting permissions to %s: %v\n", profileFile.Name(), err)
return
}
// write to file
fmt.Fprintln(profileFile, profile)
Expand Down
29 changes: 25 additions & 4 deletions cmd/capture.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ limitations under the License.
package cmd

import (
"fmt"
"os"
"strings"

"github.com/alegrey91/harpoon/internal/captor"
"github.com/alegrey91/harpoon/internal/writer"
"github.com/spf13/cobra"
)

Expand All @@ -35,13 +40,29 @@ by passing the function name symbol and the binary args.
`,
Example: " harpoon -f main.doSomething ./command arg1 arg2 ...",
Run: func(cmd *cobra.Command, args []string) {
opts := captor.CaptureOptions{

functionSymbolList := strings.Split(functionSymbols, ",")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm always worried when I see such code

And empty string will lead to a slice of 1 string with empty string in it.

So []string{""}

I would either check the length of functionSymbols before splitting, or I would skip in the for loop when strings.TrimSpace(item) == ""

I'm unsure if it can happen with your cost. But I cannot help myself to raise this to your attention

Copy link
Owner Author

Choose a reason for hiding this comment

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

Couldn't agree more with you. I'll add some checks to avoid that sometimes like so could happen. Thanks for raising the issue.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Since the --function flag is marked as required, we should never face with an empty value in input.
So for now I would skip any control and add them in the future.


captureOpts := captor.CaptureOptions{
CommandOutput: commandOutput,
LibbpfOutput: libbpfOutput,
Save: save,
Directory: directory,
}
captor.Capture(functionSymbols, args, opts)
for _, functionSymbol := range functionSymbolList {
syscalls, err := captor.Capture(functionSymbol, args, captureOpts)
if err != nil {
fmt.Printf("error capturing syscall: %v", err)
os.Exit(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bad pattern.

No os.Exit should be found outside main.go

It's outside the scope of your PR, and could be addressed in another one.

But here I would use RunE not Run because RunE signature supports returning an error.

So the os.Exit can be found in main.go only

This way you can test your code, and you will have one code to handle things

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note it could also mean you could use errors wrapping

So instead of this
fmt.Printf("error capturing syscall: %v", err)

You would use return fmt.Errorf("error capturing syscall: %w", err)

This way the fmt.Printis only present in main.go

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm removing this bad pattern from capture.go, but for the other files I'll open another PR.
I don't want to fill this PR with too many commits out of scope.

}

saveOpts := writer.WriteOptions{
Save: save,
Directory: directory,
}
if err := writer.Write(syscalls, functionSymbols, saveOpts); err != nil {
fmt.Printf("error writing syscalls for symbol %s", functionSymbol)
os.Exit(1)
}
}
},
}

Expand Down
21 changes: 14 additions & 7 deletions cmd/hunt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import (
"fmt"
"io"
"os"
"strings"

"github.com/alegrey91/harpoon/internal/captor"
meta "github.com/alegrey91/harpoon/internal/metadata"
"github.com/alegrey91/harpoon/internal/writer"
"github.com/spf13/cobra"
yaml "gopkg.in/yaml.v2"
)
Expand Down Expand Up @@ -65,15 +65,24 @@ var huntCmd = &cobra.Command{
// command builder
var captureArgs []string
captureArgs = append(captureArgs, symbolsOrigins.TestBinaryPath)
functionSymbols := strings.Join(symbolsOrigins.Symbols, ",")
opts := captor.CaptureOptions{
CommandOutput: commandOutput,
LibbpfOutput: libbpfOutput,
Save: save,
Directory: directory,
}

captor.Capture(functionSymbols, captureArgs, opts)
for _, functionSymbol := range symbolsOrigins.Symbols {
syscalls, err := captor.Capture(functionSymbol, captureArgs, opts)
if err != nil {
fmt.Printf("error capturing syscall: %v", err)
os.Exit(1)
}

saveOpts := writer.WriteOptions{
Save: save,
Directory: directory,
}
writer.Write(syscalls, functionSymbol, saveOpts)
}
}
},
}
Expand All @@ -84,8 +93,6 @@ func init() {
huntCmd.Flags().StringVarP(&harpoonFile, "file", "F", ".harpoon.yaml", "File with the result of analysis")
huntCmd.MarkFlagRequired("file")

huntCmd.Flags().StringVarP(&functionSymbols, "functions", "f", "", "Name of the function symbols to be traced")

huntCmd.Flags().BoolVarP(&commandOutput, "include-cmd-output", "c", false, "Include the executed command output")

huntCmd.Flags().BoolVarP(&libbpfOutput, "include-libbpf-output", "l", false, "Include the libbpf output")
Expand Down
82 changes: 18 additions & 64 deletions internal/captor/capture.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,13 @@ import (
"encoding/binary"
"fmt"
"os"
"path"
"path/filepath"
"strings"
"sync"
"unsafe"

"github.com/alegrey91/harpoon/internal/archiver"
embedded "github.com/alegrey91/harpoon/internal/embeddable"
"github.com/alegrey91/harpoon/internal/executor"
probes "github.com/alegrey91/harpoon/internal/probesfacade"
syscallsw "github.com/alegrey91/harpoon/internal/syscallswriter"
bpf "github.com/aquasecurity/libbpfgo"
)

Expand All @@ -36,11 +32,9 @@ type event struct {
type CaptureOptions struct {
CommandOutput bool
LibbpfOutput bool
Save bool
Directory string
}

func Capture(functionSymbols string, cmdArgs []string, opts CaptureOptions) {
func Capture(functionSymbol string, cmdArgs []string, opts CaptureOptions) ([]uint32, error) {
if !opts.LibbpfOutput {
// suppress libbpf log ouput
bpf.SetLoggerCbs(
Expand All @@ -52,13 +46,10 @@ func Capture(functionSymbols string, cmdArgs []string, opts CaptureOptions) {
)
}

functionSymbolList := strings.Split(functionSymbols, ",")

objectFile, err := embedded.BPFObject.ReadFile("output/ebpf.o")
bpfModule, err := bpf.NewModuleFromBuffer(objectFile, "ebpf.o")
if err != nil {
fmt.Printf("error loading BPF object file: %v\n", err)
os.Exit(-1)
return nil, fmt.Errorf("error loading BPF object file: %v", err)
}
defer bpfModule.Close()

Expand All @@ -68,44 +59,35 @@ func Capture(functionSymbols string, cmdArgs []string, opts CaptureOptions) {
*/
config, err := bpfModule.GetMap(bpfConfigMap)
if err != nil {
fmt.Printf("error retrieving map (%s) from BPF program: %v\n", bpfConfigMap, err)
os.Exit(-1)
return nil, fmt.Errorf("error retrieving map (%s) from BPF program: %v", bpfConfigMap, err)
}
enterFuncProbe, err := bpfModule.GetProgram(uprobeEnterFunc)
if err != nil {
fmt.Printf("error loading program (%s): %v\n", uprobeEnterFunc, err)
os.Exit(-1)
return nil, fmt.Errorf("error loading program (%s): %v", uprobeEnterFunc, err)
}
exitFuncProbe, err := bpfModule.GetProgram(uprobeExitFunc)
if err != nil {
fmt.Printf("error loading program (%s): %v\n", uprobeExitFunc, err)
os.Exit(-1)
return nil, fmt.Errorf("error loading program (%s): %v", uprobeExitFunc, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

All errors should be wrapped with %w, not %v

}
traceFunction, err := bpfModule.GetProgram(tracepointFunc)
if err != nil {
fmt.Printf("error loading program (%s): %v\n", tracepointFunc, err)
os.Exit(-1)
return nil, fmt.Errorf("error loading program (%s): %v", tracepointFunc, err)
}

bpfModule.BPFLoadObject()
for _, functionSymbol := range functionSymbolList {
offset, err := probes.AttachUProbe(cmdArgs[0], functionSymbol, enterFuncProbe)
if err != nil {
fmt.Printf("error attaching uprobe to %s: %v", functionSymbol, err)
os.Exit(-1)
}
offset, err := probes.AttachUProbe(cmdArgs[0], functionSymbol, enterFuncProbe)
if err != nil {
return nil, fmt.Errorf("error attaching uprobe to %s: %v", functionSymbol, err)
}

err = probes.AttachURETProbe(cmdArgs[0], functionSymbol, exitFuncProbe, offset)
if err != nil {
fmt.Printf("error attaching uretprobe to %s: %v", functionSymbol, err)
os.Exit(-1)
}
err = probes.AttachURETProbe(cmdArgs[0], functionSymbol, exitFuncProbe, offset)
if err != nil {
return nil, fmt.Errorf("error attaching uretprobe to %s: %v", functionSymbol, err)
}

traceLink, err := traceFunction.AttachTracepoint(tracepointCategory, tracepointName)
if err != nil {
fmt.Printf("error attaching tracepoint at event (%s:%s): %v\n", tracepointCategory, tracepointName, err)
os.Exit(-1)
return nil, fmt.Errorf("error attaching tracepoint at event (%s:%s): %v", tracepointCategory, tracepointName, err)
}
defer traceLink.Destroy()

Expand All @@ -118,17 +100,15 @@ func Capture(functionSymbols string, cmdArgs []string, opts CaptureOptions) {
baseCmd := append([]byte(baseargs), 0)
err = config.Update(unsafe.Pointer(&config_key_args), unsafe.Pointer(&baseCmd[0]))
if err != nil {
fmt.Printf("error updating map (%s) with values %d / %s: %v\n", bpfConfigMap, config_key_args, baseargs, err)
os.Exit(-1)
return nil, fmt.Errorf("error updating map (%s) with values %d / %s: %v", bpfConfigMap, config_key_args, baseargs, err)
}

// init perf buffer
eventsChannel := make(chan []byte)
lostChannel := make(chan uint64)
rb, err := bpfModule.InitPerfBuf(bpfEventsMap, eventsChannel, lostChannel, 1)
if err != nil {
fmt.Printf("error initializing map (%s) with PerfBuffer: %v\n", bpfEventsMap, err)
os.Exit(-1)
return nil, fmt.Errorf("error initializing map (%s) with PerfBuffer: %v", bpfEventsMap, err)
}

// run args that we want to trace
Expand All @@ -144,6 +124,7 @@ func Capture(functionSymbols string, cmdArgs []string, opts CaptureOptions) {
var e event
err := binary.Read(bytes.NewBuffer(data), binary.LittleEndian, &e)
if err != nil {
fmt.Fprintf(os.Stderr, "error reading event: %v\n", err)
return
}
syscalls = append(syscalls, e.SyscallID)
Expand All @@ -159,32 +140,5 @@ func Capture(functionSymbols string, cmdArgs []string, opts CaptureOptions) {
wg.Wait()
rb.Stop()

var errOut error
if opts.Save {
fileName := archiver.Convert(functionSymbolList[0])
err := os.MkdirAll(opts.Directory, os.ModePerm)
if err != nil {
fmt.Printf("error creating directory: %v\n", err)
os.Exit(-1)
}
file, err := os.Create(path.Join(opts.Directory, fileName))
if err != nil {
fmt.Printf("error creating file %s: %v\n", file.Name(), err)
os.Exit(-1)
}
defer file.Close()

if err := file.Chmod(0744); err != nil {
fmt.Printf("error setting permissions to %s: %v\n", file.Name(), err)
}
// write to file
errOut = syscallsw.Print(file, syscalls)
} else {
// write to stdout
errOut = syscallsw.Print(os.Stdout, syscalls)
}
if errOut != nil {
fmt.Printf("error printing out system calls: %v\n", errOut)
os.Exit(-1)
}
return syscalls, nil
}
45 changes: 45 additions & 0 deletions internal/writer/write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package writer

import (
"fmt"
"os"
"path"

"github.com/alegrey91/harpoon/internal/archiver"
syscallsw "github.com/alegrey91/harpoon/internal/syscallswriter"
)

type WriteOptions struct {
Save bool
Directory string
}

func Write(syscalls []uint32, functionSymbol string, opts WriteOptions) error {
var errOut error
if opts.Save {
fileName := archiver.Convert(functionSymbol)
err := os.MkdirAll(opts.Directory, os.ModePerm)
if err != nil {
return fmt.Errorf("error creating directory: %v", err)
}
file, err := os.Create(path.Join(opts.Directory, fileName))
if err != nil {
return fmt.Errorf("error creating file %s: %v", file.Name(), err)
}
defer file.Close()

if err := file.Chmod(0744); err != nil {
return fmt.Errorf("error setting permissions to %s: %v", file.Name(), err)
}
// write to file
errOut = syscallsw.Print(file, syscalls)
} else {
// write to stdout
errOut = syscallsw.Print(os.Stdout, syscalls)
}
if errOut != nil {
return fmt.Errorf("error printing out system calls: %v", errOut)
}

return nil
}