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

enhance(scripts/build/termux_step_massage): check for non-position-independent executables #23604

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertkirkman
Copy link
Contributor

@robertkirkman robertkirkman commented Mar 4, 2025

  • a check-pie.sh exists, but it is an isolated tool, and is not connected to CI.

#!/bin/sh
# check-pie.sh - script to detect non-PIE binaries (which does not work on Android)
. $(dirname "$(realpath "$0")")/properties.sh
cd ${TERMUX_PREFIX}/bin
for file in *; do
if readelf -h $file 2>/dev/null | grep -q 'Type:[[:space:]]*EXEC'; then
echo $file
fi
done

  • [Bug]: Can't execute the Package x11/simulide #23598 could have been prevented, in retrospect, if a long time ago when simulide was last built, check-pie.sh had been connected to CI.

  • This is an attempt to try to copy and paste the contents of check-pie.sh into the symbol checks block of termux_step_massage.sh, in order to possibly enable it

Effects

INFO: Running symbol checks on 1 files with nproc=32
INFO: Done ... 0s
INFO: Found non-position-independent executables
INFO: Showing result
ERROR: ./bin/simulide is a non-position-independent executable
INFO: Done ... 0s
ERROR: Refer above
INFO: Running symbol checks on 1 files with nproc=32
INFO: Done ... 1s
termux - build of 'simulide' done
  • has been tested minimally in both $TERMUX_ON_DEVICE_BUILD=true mode and $TERMUX_ON_DEVICE_BUILD=false mode, but has not been heavily tested with all packages yet. Because check-pie.sh has never been connected to CI, there is a chance it might produce false positives with some unknown packages, in which case a TERMUX_PKG_NO_PIE_FILES variable would be necessary to implement, for those packages. This PR serves to float the idea to check whether this is considered worthwhile enough to try.

@robertkirkman robertkirkman requested a review from Grimler91 as a code owner March 4, 2025 23:44
…dependent executables

- a [`check-pie.sh`](https://github.com/termux/termux-packages/blob/5ed8471923a57fab306148c2cdef88df49550b68/scripts/bin/check-pie.sh) exists, but it is an isolated tool, and is not connected to CI.

https://github.com/termux/termux-packages/blob/5ed8471923a57fab306148c2cdef88df49550b68/scripts/bin/check-pie.sh#L1-L12

- termux#23598 could have been prevented, in retrospect, if a long time ago when `simulide` was last built, `check-pie.sh` had been connected to CI.

- This is an attempt to try to copy and paste the contents of `check-pie.sh` into the symbol checks block of `termux_step_massage.sh`, in order to possbly enable it

- effects:

  - before termux@e462a5d:

```
INFO: Running symbol checks on 1 files with nproc=32
INFO: Done ... 0s
INFO: Found non-position-independent executables
INFO: Showing result
ERROR: ./bin/simulide is a non-position-independent executable
INFO: Done ... 0s
ERROR: Refer above
```

  - after termux@e462a5d:

```
INFO: Running symbol checks on 1 files with nproc=32
INFO: Done ... 1s
termux - build of 'simulide' done
```

- **has been tested minimally in both `$TERMUX_ON_DEVICE_BUILD=true` mode and `$TERMUX_ON_DEVICE_BUILD=false` mode, but has not been heavily tested with all packages yet**. Because `check-pie.sh` has never been connected to CI, there is a chance it might produce **false positives** with some unknown packages, in which case a `TERMUX_PKG_NO_PIE_FILES` variable would be necessary to implement, for those packages. This PR serves to float the idea to check whether this is considered worthwhile enough to try.
@robertkirkman
Copy link
Contributor Author

hmm, I copied and pasted the "ERROR: Done" message from the block above that (1a4356d#diff-16d956212adc88d9e2d11e83f8f76549851ba7df6c22552146fd376176cb969cR328), but all of the other "Done" messages start with "INFO", not "ERROR". I will assume that is a minor typo and correct it.

@robertkirkman
Copy link
Contributor Author

robertkirkman commented Mar 6, 2025

I have been keeping this in my local folder while building some packages, to look for false positives, and I have found one false positive.

  • main/golang

When building it with check-pie enabled, I saw this:

INFO: Found non-position-independent executables
INFO: Showing result
ERROR: ./lib/go/src/debug/dwarf/testdata/line-clang-dwarf5.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/line-clang.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/line-gcc-dwarf5.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/line-gcc.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/line-gcc-zstd.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/ranges.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/rnglistx.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/split.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/dwarf/testdata/typedef.elf is a non-position-independent executable
ERROR: ./lib/go/src/debug/elf/testdata/gcc-386-freebsd-exec is a non-position-independent executable
ERROR: ./lib/go/src/debug/elf/testdata/gcc-amd64-linux-exec is a non-position-independent executable
ERROR: ./lib/go/src/runtime/pprof/testdata/test32 is a non-position-independent executable
ERROR: ./lib/go/src/runtime/pprof/testdata/test32be is a non-position-independent executable
ERROR: ./lib/go/src/runtime/pprof/testdata/test64 is a non-position-independent executable
ERROR: ./lib/go/src/runtime/pprof/testdata/test64be is a non-position-independent executable
INFO: Done ... 0s
ERROR: Refer above

This happens because, these are real non-position-independent executables, but they are not actually stored into the package, ever since 2558c84 10 years ago, so they are not intended to be used on-device.

But since they are removed during termux_step_post_massage() which runs after termux_step_massage(), they show up in the check.

After seeing that happen, I checked every single other termux_step_post_massage() in the repository to look for any that seemed like they could also cause false positives in a similar way, and I haven't found any.

I think a pretty good way to resolve this would be to just move the command that removes the testdata folders from termux_step_post_massage() to termux_step_make_install(), hopefully?

golang is in the process of being bumped, so I will ask about this there also, or wait until after golang bump is finished before adding that change to this PR.

This is the change I have tested:

--- a/packages/golang/build.sh
+++ b/packages/golang/build.sh
@@ -51,8 +51,6 @@ termux_step_make_install() {
 	cp pkg/include/* $TERMUX_GODIR/pkg/include/
 	cp -Rf lib/* $TERMUX_GODIR/lib
 	cp -Rf misc/ $TERMUX_GODIR/
-}
-
-termux_step_post_massage() {
-	find . -path '*/testdata*' -delete
+	# testdata directories are not needed on the installed system
+	find $TERMUX_GODIR/src -path '*/testdata*' -delete
 }

@robertkirkman robertkirkman marked this pull request as draft March 6, 2025 08:15
@agnostic-apollo
Copy link
Member

You can create a step with an inner function like following where packages can filter the paths that should be searched for PIEs. I doubt you will be able to override make step for all packages that have such non-PIEs files. Like termux_step_check_pie() and termux_step_check_pie__in_path().

@robertkirkman
Copy link
Contributor Author

robertkirkman commented Mar 6, 2025

You can create a step with an inner function like following where packages can filter the paths that should be searched for PIEs.

Ok thank you, yes that would be better for code organization and flexibility, because I think it is technically not a type of symbol, it is just detected using readelf like symbols are. I wasn't sure whether it should be a separate step or not so now I will make it one.

I doubt you will be able to override make step for all packages that have such non-PIEs files.

I expected to see much more false positives, but I am pretty surprised because I have tested quite a few packages with it now and so far the only false positive I have seen yet has been golang.

I have not yet seen any true positives (packages still containing real non-position-independent executables that made it all the way into the app), but based on what happened to simulide, it is possible that a future new package or a future upstream change of an existing package could accidentally add -no-pie into its linker arguments at any time, so it seems worth checking for.

There are a few fully statically linked executables (pandoc, ncdu2, zls, zig and any Zig programs that zig compiles on-device), which would come up as false positives naturally since I think they technically are non-position-independent executables, but for some reason they still work. I am not completely familiar with the reasons why the fully statically linked executables work, but they do at least for now. but that is why I have the check placed inside a $TERMUX_PACKAGE_LIBRARY=bionic block (in my opinion the fully statically linked executables should probably be called $TERMUX_PACKAGE_LIBRARY=musl not $TERMUX_PACKAGE_LIBRARY=bionic, that just isn't set up currently I think).

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.

2 participants