link/executable: lazy load symbol table #991
Conversation
mmat11
force-pushed
the
matt/lazy-load-symbols
branch
2 times, most recently
from
d173315 to
056a6af
Compare
| @@ -34,6 +35,9 @@ type Executable struct { | |||
| path string | |||
| // Parsed ELF and dynamic symbols' addresses. | |||
| addresses map[string]uint64 | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mmat11
force-pushed
the
matt/lazy-load-symbols
branch
from
056a6af to
dd22162
Compare
| @@ -83,32 +86,10 @@ func OpenExecutable(path string) (*Executable, error) { | |||
| return nil, fmt.Errorf("path cannot be empty") | |||
| } | |||
|
|
|||
| f, err := os.Open(path) | |||
There was a problem hiding this comment.
We don't return an error when the given path doesn't exist, which seems suboptimal. We could stat the file, but then we'd get a TOCTOU because we later on need to Open and the binary might have changed. Does that matter? Seems like the tracefs api only deals with paths as well, which implies it has the same problem. Is there a way to avoid paths with tracefs?
One workaround would be to still Open here and put *os.File into Executable. But then we leak *os.File. Do we need Executable.Close()?
There was a problem hiding this comment.
I thought about this yesterday :D
We don't return an error when the given path doesn't exist, which seems suboptimal. We could stat the file, but then we'd get a TOCTOU because we later on need to Open and the binary might have changed. Does that matter? Seems like the tracefs api only deals with paths as well, which implies it has the same problem. Is there a way to avoid paths with tracefs?
I think pmu has the same problem (perf event is opened on the given executable path), so if the binary changes it will break anyway (or, if the check has never been done, it will fail regardless)
There was a problem hiding this comment.
Hm, okay. And there is no API that takes an FD?
I'd still prefer to stat here in addition to your current approach so that we return an error early.
There was a problem hiding this comment.
Hm, okay. And there is no API that takes an FD?
https://man7.org/linux/man-pages/man2/perf_event_open.2.html this is the API we are using :D (ctrl+f uprobe_path)
I'd still prefer to stat here in addition to your current approach so that we return an error early.
ok, I'll add that check early
link/uprobe_test.go
Outdated
| ex, err := OpenExecutable("/bin/bash") | ||
| c.Assert(err, qt.IsNil) | ||
| // Addresses must be empty, will be lazy loaded. | ||
| c.Assert(ex.addresses, qt.CmpEquals(), map[string]uint64{}) |
There was a problem hiding this comment.
Huh, isn't CmpEquals() the same as Equals?
There was a problem hiding this comment.
CmpEquals uses go-cmp's .Diff() while Equals uses ==
There was a problem hiding this comment.
Ah, TIL! Maybe https://pkg.go.dev/github.com/frankban/[email protected]#DeepEquals is more idiomatic then?
mmat11
force-pushed
the
matt/lazy-load-symbols
branch
from
dd22162 to
cd7bd95
Compare
Signed-off-by: Mattia Meleleo <[email protected]>
mmat11
force-pushed
the
matt/lazy-load-symbols
branch
from
cd7bd95 to
e176c3b
Compare
|
Thanks for the quick turn around! I've reworded your commit message to include context from the issue and the PR review. |
|
You people are awesome 👏 Thanks a lot for the quick change! |
#987