-
Notifications
You must be signed in to change notification settings - Fork 399
python: read Py_Version when filename lacks a version #1124
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
baa8bd7
python: fall back to Py_Version when filename lacks a version
dkratunov ca3ce00
review feedback
dkratunov 40932ab
Merge upstream/main into pyversion-lookup
dkratunov 5f0d1d6
python: avoid int-to-uint16 narrowing in pythonVer
dkratunov 1f4ef25
Merge branch 'main' into pyversion-lookup
dkratunov 06a7dad
python: address codeql atoi
dkratunov f07186a
skip versionHex explicit check
dkratunov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "path" | ||
| "reflect" | ||
| "regexp" | ||
| "slices" | ||
|
|
@@ -48,6 +49,23 @@ func pythonVer(major, minor uint16) uint16 { | |
| return major*0x100 + minor | ||
| } | ||
|
|
||
| func readPyVersionHex(ef *pfelf.File) (major uint8, minor uint8, err error) { | ||
| // Py_Version is referenced in CPython internals for versioned Python binaries. | ||
| // https://github.com/python/cpython/blob/v3.11.0/Doc/c-api/apiabiversion.rst | ||
| addr, err := ef.LookupSymbolAddress("Py_Version") | ||
|
dkratunov marked this conversation as resolved.
|
||
| if err != nil { | ||
| return 0, 0, err | ||
| } | ||
| rm := ef.GetRemoteMemory() | ||
| versionHex := rm.Uint32(libpf.Address(addr)) | ||
| major = uint8((versionHex >> 24) & 0xff) | ||
| minor = uint8((versionHex >> 16) & 0xff) | ||
| if major == 0 { | ||
| return 0, 0, fmt.Errorf("invalid Py_Version 0x%x", versionHex) | ||
| } | ||
| return major, minor, nil | ||
| } | ||
|
|
||
| //nolint:lll | ||
| type pythonData struct { | ||
| version uint16 | ||
|
|
@@ -721,19 +739,36 @@ func decodeStub(ef *pfelf.File, memoryBase libpf.SymbolValue, | |
|
|
||
| func Loader(ebpf interpreter.EbpfHandler, info *interpreter.LoaderInfo) (interpreter.Data, error) { | ||
| mainDSO := false | ||
| major := uint16(0) | ||
| minor := uint16(0) | ||
| matches := libpythonRegex.FindStringSubmatch(info.FileName()) | ||
| if matches == nil { | ||
| mainDSO = true | ||
| matches = pythonRegex.FindStringSubmatch(info.FileName()) | ||
| if matches == nil { | ||
| } | ||
| if matches == nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we move |
||
| if !strings.HasPrefix(path.Base(info.FileName()), "python") { | ||
| return nil, nil | ||
| } | ||
| } else { | ||
| majorValue, _ := strconv.ParseUint(matches[1], 10, 16) | ||
| minorValue, _ := strconv.ParseUint(matches[2], 10, 16) | ||
| major = uint16(majorValue) | ||
| minor = uint16(minorValue) | ||
| } | ||
|
|
||
| ef, err := info.GetELF() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if major == 0 { | ||
| majorFromSym, minorFromSym, versionErr := readPyVersionHex(ef) | ||
| if versionErr != nil { | ||
| return nil, nil | ||
| } | ||
| major = uint16(majorFromSym) | ||
| minor = uint16(minorFromSym) | ||
| } | ||
|
|
||
| if mainDSO { | ||
| var needed []string | ||
|
|
@@ -749,9 +784,7 @@ func Loader(ebpf interpreter.EbpfHandler, info *interpreter.LoaderInfo) (interpr | |
| } | ||
|
|
||
| var pyruntimeAddr, autoTLSKey libpf.SymbolValue | ||
| major, _ := strconv.ParseUint(matches[1], 10, 16) | ||
| minor, _ := strconv.ParseUint(matches[2], 10, 16) | ||
| version := pythonVer(uint16(major), uint16(minor)) | ||
| version := pythonVer(major, minor) | ||
|
|
||
| minVer := pythonVer(3, 6) | ||
| maxVer := pythonVer(3, 14) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a coredump test hitting this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not super easy for arbitrary contributors to add core dumps afaict. Do I not need s3 put access first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a google drive link (or similar shareable blob store) and share it with one of the maintainers giving them read access, @florianl has been very helpful with uploading coredumps for the ruby tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A full coredump test might be a bit much to test this function. Therefore it is fine for me to continue and merge it without. A more simpler way to test
readPyVersionHex()could be to have a base64 encoded data string, likehelloworldBinin testsupport, and add a regular unit test.