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

SEA created following Node.js documentation doesn't work #76

Closed
targos opened this issue Feb 22, 2023 · 8 comments
Closed

SEA created following Node.js documentation doesn't work #76

targos opened this issue Feb 22, 2023 · 8 comments

Comments

@targos
Copy link
Member

targos commented Feb 22, 2023

Doing exactly as documented in https://nodejs.org/api/single-executable-applications.html

On macOS arm64

$ cat hello.js
console.log(`Hello, ${process.argv[2]}!`);

$ ./hello -v
v19.7.0

$ npx postject hello NODE_JS_CODE hello.js \
    --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 \
    --macho-segment-name NODE_JS

$ ./hello
[1]    20992 killed     ./hello

$ ./hello world
[1]    21015 killed     ./hello world

On Linux x64

$ cat hello.js
console.log(`Hello, ${process.argv[2]}!`);

$ ./hello -v
v19.7.0

$ npx postject hello NODE_JS_CODE hello.js \
    --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note'
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note.100'
Can't find string offset for section name '.note.100'

$ ./hello
[1]    2517236 segmentation fault (core dumped)  ./hello

$ ./hello world
[1]    2517265 segmentation fault (core dumped)  ./hello world
@RaisinTen
Copy link
Contributor

Regarding the macOS issue, could you try running codesign --sign - hello and then try again? I think it segfaults because the binary isn't signed anymore after postject processes it and having executables signed is a hard requirement on arm64.

@targos
Copy link
Member Author

targos commented Feb 22, 2023

Regarding the macOS issue, could you try running codesign --sign - hello and then try again?

That works! I think it should be documented somewhere.

RaisinTen added a commit to RaisinTen/node that referenced this issue Feb 22, 2023
We didn't catch this in nodejs#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor

Done - nodejs/node#46764!

@RaisinTen
Copy link
Contributor

I'm able to reproduce the Linux issue. Just wondering why we weren't able to catch this when the change was in a PR state. @targos do we use some non-default configure flags to build Node.js for official releases? Also, what compiler did we use for the release?

@targos
Copy link
Member Author

targos commented Feb 23, 2023

Based on https://ci-release.nodejs.org/job/iojs+release/9167/nodes=rhel8-x64-release/consoleFull

System: RHEL 8.7
Compiler: gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-16)
Configure:

python3 ./configure \
	--prefix=/ \
	--dest-cpu=x64 \
	--tag= \
	--release-urlbase=https://nodejs.org/download/release/ \
	--download-path=/home/iojs/node-icu/ --verbose --download=all --with-intl=full-icu

Compile:

make install DESTDIR=node-v19.7.0-linux-x64 V=0 PORTABLE=1
make -C out BUILDTYPE=Release V=0

Edit: found the release build for 19.7.0

@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 25, 2023

@targos I wasn't able to repro this locally when I built node using the same options on an x64 ubuntu. Maybe this has something to do with the compiler / system where the build took place. GDB tells me that the crash took place somewhere inside:

0x0000000000c7df55 in (anonymous namespace)::GetSingleExecutableCode(v8::FunctionCallbackInfo<v8::Value> const&) ()

This is the topmost frame in the stack trace, so I would need some more info.

(I have a hunch that this crash source is the same as #70 but I don't have a way to confirm without having access to the system.)

I'll open an issue asking for access in the build repo - nodejs/build#3207.

RaisinTen added a commit to nodejs/node that referenced this issue Feb 26, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RaisinTen added a commit that referenced this issue Feb 27, 2023
The program headers base address values returned by `getauxval(AT_PHDR)`
and `dl_iterate_phdr()` are identical only on
`g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0`. However, the values are
totally different on `clang version 10.0.0-4ubuntu1` and
`g++ (GCC) 8.5.0 20210514 (Red Hat 8.5.0-16)`.
Since the `dl_iterate_phdr()` approach seems to work for all the 3
compilers, I think we should proceed with that.

Fixes: #70
Refs: #76
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor

I've confirmed that #77 fixes the issue.

RaisinTen added a commit that referenced this issue Mar 3, 2023
* fix: crash in `postject_find_resource()` on Linux

The program headers base address values returned by `getauxval(AT_PHDR)`
and `dl_iterate_phdr()` are identical only on
`g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0`. However, the values are
totally different on `clang version 10.0.0-4ubuntu1` and
`g++ (GCC) 8.5.0 20210514 (Red Hat 8.5.0-16)`.
Since the `dl_iterate_phdr()` approach seems to work for all the 3
compilers, I think we should proceed with that.

Fixes: #70
Refs: #76
Signed-off-by: Darshan Sen <raisinten@gmail.com>

* chore: remove unnecessary if block

Signed-off-by: Darshan Sen <raisinten@gmail.com>

* fix: only iterate the main executable program headers

The resource gets injected in the main executable, so there is no need
to iterate the other shared libraries that are loaded by the program.
This also resolves a security concern.

Refs: #77 (review)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

* chore: shorten change

Signed-off-by: Darshan Sen <raisinten@gmail.com>

* test: add comment about _GNU_SOURCE

Signed-off-by: Darshan Sen <raisinten@gmail.com>

---------

Signed-off-by: Darshan Sen <raisinten@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Mar 13, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Mar 13, 2023
PR-URL: #46934
Fixes: nodejs/postject#76
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Mar 14, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Mar 14, 2023
PR-URL: #46934
Fixes: nodejs/postject#76
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos
Copy link
Member Author

targos commented Mar 16, 2023

I confirm that the binary created with postject@1.0.0-alpha.5 and Node.js 19.8.1 works.

There are still warnings printed, though:

$ npx postject@latest hello NODE_JS_CODE hello.js \
    --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2
Start injection of NODE_JS_CODE in hello...
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note'
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note.100'
warning: Can't find string offset for section name '.note.100'
💉 Injection done!

danielleadams pushed a commit to nodejs/node that referenced this issue Apr 11, 2023
PR-URL: #46934
Fixes: nodejs/postject#76
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Nov 10, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
We didn't catch this in nodejs/node#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
We didn't catch this in nodejs/node#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 a pull request may close this issue.

2 participants