Skip to content

Fix invalid XNU binaries generated by apelink in some edge cases #1106

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 2 commits into from
Feb 23, 2024

Conversation

dfyz
Copy link
Contributor

@dfyz dfyz commented Feb 11, 2024

I was experimenting with aarch64-only APE binaries that don't contain any embedded loaders. You can generate one with:

> cat tmp.c
#include <stdio.h>

int main() {
    puts("wow");
}
> aarch64-unknown-cosmo-cc -O2 -o tmp.aarch64 tmp.c
> apelink -o tmp.com tmp.aarch64

When I copied the resulting binary to an M1 machine and tried to run it, I got the following confusing error:

> ./tmp.com
./tmp.com: line 19: syntax error near unexpected token `fi'
./tmp.com: line 19: `fi'

The details of what's going on and how this it was fixed are in the commit message. The generated binaries still don't work because apparently you can't assimilate on Apple Silicon, but the error message is clearer:

> ./tmp.com
./tmp.com: this ape program lacks arm64 support

While I'm at it, also fix slightly outdated apelink documentation.

dfyz added 2 commits February 11, 2024 02:53
A shell will fail with a syntax error on an empty `if` or `else` body.
That is, neither of these is allowed:

    # Empty `if`
    if [ ... ]; then
    fi

    # Empty `else`
    if [ ... ]; then
    ...
    else
    fi

There were two places where `apelink` could generate problematic `if`'s:

1. The XNU shell generation for aarch64 binaries when no loaders (either
   binary or source) are provided. They can't assimilate, so the resulting
   `else` body becomes empty.
   There is actually a code path guarded by the `gotsome` variable that
   inserts an extra `true` in this case, but the variable was never
   initialized, so in practice this code path didn't activate in my
   tests. This is fixed by initializing the variable.
2. The loader extraction code when no loaders are provided and XNU
   support is requested. This is fixed by adding a simliar code path
   that prevents an empty body from being generated.
The `-s` option changed its meaning, but the docs weren't updated.
@ghaerr
Copy link
Contributor

ghaerr commented Feb 11, 2024

Looks good!

There is actually a code path guarded by the gotsome variable that
inserts an extra true in this case, but the variable was never
initialized, so in practice this code path didn't activate in my
tests.

I'm a little surprised there was no compiler warning of use of an uninitialized variable during static flow analysis... was that perhaps inadvertently turned off?

@dfyz
Copy link
Contributor Author

dfyz commented Feb 11, 2024

I'm a little surprised there was no compiler warning of use of an initialized variable during static flow analysis... was that perhaps inadvertently turned off?

That's a very good question. I just did some research, and I believe the static analysis is on, it's just that gcc has a known issue with this specific code pattern (in particular, this comment is very similar to what we have with gotsome).

If I remove all lines that assign to gotsome, I get a compilation error, as expected:

> make -j o//tool/build/apelink.com.dbg
build/bootstrap/compile.com -V9 -M2048m -P8192  -AMKDEPS -L320 build/bootstrap/mkdeps.com -o o//depend -s -r o// @o//srcs.txt @o//hdrs.txt @o//incs.txt
    108,453⏰    105,779⏳    14,924k   9,032iop build/bootstrap/mkdeps.com -o o//depend -s -r o// @o//srcs.txt @o//hdrs.txt @o//incs.txt
tool/build/apelink.c: In function ‘main’:
tool/build/apelink.c:2130:10: error: ‘gotsome’ may be used uninitialized [-Werror=maybe-uninitialized]
 2130 |       if (!gotsome) {
      |          ^
cc1: all warnings being treated as errors

@ghaerr
Copy link
Contributor

ghaerr commented Feb 11, 2024

Thanks for the research. That's good to know that -Wuninitialized doesn't always work, even in some fairly simple cases!

@dfyz
Copy link
Contributor Author

dfyz commented Feb 23, 2024

@jart Would you be interested in merging this? I don't think creating pure aarch64 binaries is a very common use case, but the fixes themselves, I think, are relatively uncontroversial (especially the uninitialized variable fix).

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

Yes I am interested in merging this. Another outstanding change from you. Approved. Come hang out with us on Discord by the way, if you haven't joined already. https://discord.gg/FwAVVu7eJ4

@jart jart merged commit 99f0491 into jart:master Feb 23, 2024
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