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.

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

SystemSan: use tgkill on precise pid #8615

Merged
merged 2 commits into from
Oct 16, 2022

Conversation

catenacyber
Copy link
Contributor

cc @oliverchang @alan32liu

This patch is meant for tgkill to use the right thread, so that we get the right stack trace every time

Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm finding this a bit hard to follow though. Can you please explain what this is doing and why the previous approach (just by tracking g_root_pid) wasn't sufficient?

infra/experimental/SystemSan/SystemSan.cpp Outdated Show resolved Hide resolved
ThreadParent() : pid(0) {};
ThreadParent(pid_t pid) : pid(pid) {}
};
// Map of the created PID/TID to its creator PID/TID or 0 if
Copy link
Collaborator

Choose a reason for hiding this comment

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

or 0 if ? please finish the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better comment put

infra/experimental/SystemSan/SystemSan.cpp Show resolved Hide resolved
@catenacyber
Copy link
Contributor Author

I'm finding this a bit hard to follow though. Can you please explain what this is doing and why the previous approach (just by tracking g_root_pid) wasn't sufficient?

With the previous approach, we did not get the right stack trace (ie the one with LLVMFuzzerTestOneInput) when the fuzzing was multi-threading because we were always sending the signal to the first thread.

I tried to improve the comments... Let me know what you think.

@catenacyber
Copy link
Contributor Author

By the way, for golang, the problem is that libFuzzer does not know how to print a stack trace of a Golang program

A way to get a good stack trace is to do a bit like for coverage :

  • build a standalone golang program (ie without libFuzzer) that takes a filename as single argument
  • run ./SystemSan ./repro_pure_go_fuzz testcase

So, we can see

===BUG DETECTED: Arbitrary file open===
===File opened: /?, flags = 524288,O_RDONLY===
SIGABRT: abort
PC=0x40362e m=0 sigcode=18446744073709551610

goroutine 1 [syscall]:
syscall.Syscall6(0xc000022370?, 0x2?, 0x8000000000000000?, 0x0?, 0x0?, 0x0?, 0x15864e547b1f?)
	/root/.go/src/syscall/syscall_linux.go:90 +0x36 fp=0xc0001253d0 sp=0xc000125348 pc=0x482256
syscall.openat(0x8?, {0xc000022370?, 0x1?}, 0xa?, 0x0)
	/root/.go/src/syscall/zsyscall_linux_amd64.go:68 +0x94 fp=0xc000125448 sp=0xc0001253d0 pc=0x481714
syscall.Open(...)
	/root/.go/src/syscall/syscall_linux.go:232
os.openFileNolog({0xc000022370, 0x2}, 0x0, 0x0)
	/root/.go/src/os/file_unix.go:216 +0x9b fp=0xc000125490 sp=0xc000125448 pc=0x493f1b
os.OpenFile({0xc000022370, 0x2}, 0x0, 0x8?)
	/root/.go/src/os/file.go:337 +0x45 fp=0xc0001254c8 sp=0xc000125490 pc=0x493745
os.Open(...)
	/root/.go/src/os/file.go:317
... more stack trace of the fuzz target

This pure golang program is

package main

import (
	"fmt"
	"io/ioutil"
	"os"

	"fuzzing package"
)

func main() {
	data, err := ioutil.ReadFile(os.Args[1])
	if err != nil {
		fmt.Printf("Failed to read corpus file", err)
                return
	}
	fuzz.FuzzFunction(data)
}

@catenacyber
Copy link
Contributor Author

Ping on this ?

I pushed a new version that I could successfully test with all

./SystemSan ./target -dict=vuln.dict -fork=2
./SystemSan ./target -dict=vuln.dict
./SystemSan ./target_file -dict=vuln.dict -fork=2
./SystemSan ./target_file -dict=vuln.dict

Previously, the stack trace obtained with fork=2 as in https://oss-fuzz.com/testcase-detail/5418145183825920 does not get the right stack trace

The subtlety was that for shell corruption, we have to kill the parent (when for arbitrary file open or other normal cases, we kill the current pid/tid)

Copy link
Contributor

@DonggeLiu DonggeLiu left a comment

Choose a reason for hiding this comment

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

Approved! Thanks!

@DonggeLiu DonggeLiu merged commit f5f128e into google:master Oct 16, 2022
onionpsy pushed a commit to CodeIntelligenceTesting/oss-fuzz that referenced this pull request Oct 26, 2022
This patch is meant for `tgkill` to use the right thread, so that we get
the right stack trace every time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants