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

Support long PATH values in erlexec #9331

Open
wants to merge 6 commits into
base: maint
Choose a base branch
from

Conversation

HoloRin
Copy link
Contributor

@HoloRin HoloRin commented Jan 22, 2025

Previously a finite 10240 buffer was used. Long paths can fail semi-silently, while long paths that already contain the erlang bindir at the end can cause a crash.

Reproducible with the following bash script:

#!/usr/bin/env bash
set -euo pipefail

ERL_PATH="$(erl -eval 'io:format("~s~n", [os:getenv("PATH")])' -s init stop -noshell)"

IFS=':' read -ra ERL_PATH_COMPONENTS <<< "$ERL_PATH"

BINDIR="${ERL_PATH_COMPONENTS[0]}"
OTHER_DIR="${ERL_PATH_COMPONENTS[1]}"

while [ ${#PATH} -lt 10240 ]; do
    PATH="${PATH}:${OTHER_DIR}"
done
PATH="${PATH}:${PATH}:${BINDIR}"

erl -eval 'io:format("hello~n", [])' -s init stop -noshell

Output:

% ./repro.sh
./repro.sh: line 19: 98643 Bus error: 10           erl -eval 'io:format("hello~n", [])' -s init stop -noshell

Copy link
Contributor

github-actions bot commented Jan 22, 2025

CT Test Results

    3 files    143 suites   49m 0s ⏱️
1 598 tests 1 549 ✅ 49 💤 0 ❌
2 342 runs  2 268 ✅ 74 💤 0 ❌

Results for commit 9ac7bcf.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi
Copy link
Contributor

Hello! Thanks for the fix! Can you add a test to https://github.com/erlang/otp/blob/master/erts/test/erlexec_SUITE.erl ?

@HoloRin HoloRin force-pushed the erlexec/long-path-fix branch from af3cca6 to 07339e8 Compare January 22, 2025 15:01
@HoloRin
Copy link
Contributor Author

HoloRin commented Jan 22, 2025

@garazdawi how do I run just erlexec_SUITE? I am getting a hard crash of my whole macbook when I run cd erts && make test

@garazdawi
Copy link
Contributor

Try make system_test ARGS="-suite erlexec_SUITE" from the top. There are more indepth descriptions here: https://github.com/erlang/otp/blob/master/HOWTO/TESTING.md

@garazdawi garazdawi self-assigned this Jan 23, 2025
@garazdawi garazdawi added team:VM Assigned to OTP team VM fix labels Jan 23, 2025
@HoloRin HoloRin force-pushed the erlexec/long-path-fix branch from d666721 to c4257e7 Compare January 23, 2025 14:44
@ericmj
Copy link
Contributor

ericmj commented Jan 23, 2025

@garazdawi We added some test cases for long PATHs and various combinations of the bin directory location in the PATH.

HoloRin and others added 2 commits January 24, 2025 08:46
Previously a finite 10240 buffer was used. Long paths can fail
semi-silently, while long paths that already contain the erlang bindir
at the end can cause a crash.

Co-authored-by: Eric Meadows-Jönsson <[email protected]>
Co-authored-by: Rin Kuryloski <[email protected]>
Co-authored-by: Bella Bai <[email protected]>
@HoloRin HoloRin force-pushed the erlexec/long-path-fix branch from 52cd2da to 71e9483 Compare January 24, 2025 07:46
@garazdawi
Copy link
Contributor

Great! Thanks! Putting into testing over the weekend.

@garazdawi garazdawi added the testing currently being tested, tag is used by OTP internal CI label Jan 24, 2025
@jhogberg
Copy link
Contributor

erlexec_SUITE:long_path_env/1 goes sideways on all tested platforms, failing on line 457:

  447: long_path_env(Config) when is_list(Config) ->
  448:     OriginalPath = os:getenv("PATH"),
  449:     [BinPath, RestPath] = string:split(OriginalPath, ":"),
  450:     LongPath = lists:duplicate(10240, "x"),
  451:     ExpectedPath = OriginalPath ++ ":" ++ LongPath,
  452:     {ok, [[PName]]} = init:get_argument(progname),
  453:     Cmd = PName ++ " -noshell -eval 'io:format(\"~ts\", [os:getenv(\"PATH\")]),erlang:halt()'",
  454: 
  455:     os:putenv("PATH", OriginalPath ++ ":" ++ LongPath ++ ":" ++ BinPath),
  456:     Output1 = os:cmd(Cmd),
  457:     true = string:equal(ExpectedPath, Output1),
  458: 
  459:     os:putenv("PATH", ExpectedPath),
  460:     Output2 = os:cmd(Cmd),
  461:     true = string:equal(ExpectedPath, Output2),
  462: 
  463:     os:putenv("PATH", RestPath ++ ":" ++ LongPath),
  464:     Output3 = os:cmd(Cmd),
  465:     true = string:equal(ExpectedPath, Output3),
  466: 
  467:     ok.

@garazdawi
Copy link
Contributor

The test assumes that the BinPath is the first segment of the PATH, but it does not have to be. Not sure we have a function to locate which path segment an executable is located in...

@garazdawi garazdawi added waiting waiting for changes/input from author and removed testing currently being tested, tag is used by OTP internal CI labels Jan 27, 2025
@HoloRin
Copy link
Contributor Author

HoloRin commented Jan 27, 2025

The test assumes that the BinPath is the first segment of the PATH, but it does not have to be. Not sure we have a function to locate which path segment an executable is located in...

Our understanding of the code existing behavior in erlexec is that the BinPath prepended to the PATH, and any duplicates found later in the path are dropped. In the test, this has already happened once, so BinPath is at the head of the PATH. However, I suppose that leaves open a possibility of an implementation passing this test where that's not actually the case. We can strengthen the test.

@garazdawi
Copy link
Contributor

I don't know why, but when running tests through ts (Which is used by all our interal tests, but not by github actions) the current test is passed as the first argument in the path. Something like this:

erl -env PATH "/ldisk/daily_build/28_master-opu_o.2025-01-26_21/test/emulator_test:/ldisk/daily_build/28_master-opu_o.2025-01-26_21/otp/lib/erlang/erts-16.0/bin:/"

which makes the "emulator_test" be the first in the path. You should be able to reproduce it like this:

export ERL_TOP=`pwd`
./otp_build -a
make release_tests
cd release/tests/test_server
PATH=$ERL_TOP/bin/:$PATH erl -noshell -eval 'ts:install(), ts:run(system, erlexec_SUITE, long_path_env, [batch]).' -s init stop

ericmj and others added 2 commits January 27, 2025 14:46
The PATH should not end with `:`.

Co-authored-by: Rin Kuryloski <[email protected]>
Co-authored-by: Rin Kuryloski <[email protected]>
@@ -554,38 +554,54 @@ int main(int argc, char **argv)

if (s == NULL) {
erts_snprintf(tmpStr, sizeof(tmpStr),
"%s" PATHSEP "%s" DIRSEP "bin" PATHSEP, bindir, rootdir);
"%s" PATHSEP "%s" DIRSEP "bin", bindir, rootdir);
Copy link
Contributor

Choose a reason for hiding this comment

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

We removed the trailing the PATHSEP here since the PATH should never end with : afaik.

@ericmj
Copy link
Contributor

ericmj commented Jan 27, 2025

@garazdawi Thanks for giving us a to reproduce the error. Since ts sets a different a PATH it's hard to write tests for both ts and make system_test but we have done a best effort.

@CLAassistant
Copy link

CLAassistant commented Jan 27, 2025

CLA assistant check
All committers have signed the CLA.

@garazdawi garazdawi added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author labels Jan 28, 2025
erts/etc/common/erlexec.c Outdated Show resolved Hide resolved
erts/test/erlexec_SUITE.erl Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants