Skip to content

[Fix] check invalid NVM_DIR in install script - #3861

Draft
AlightSoulmate wants to merge 2 commits into
nvm-sh:masterfrom
AlightSoulmate:fix-install-invalid-nvm-dir
Draft

[Fix] check invalid NVM_DIR in install script#3861
AlightSoulmate wants to merge 2 commits into
nvm-sh:masterfrom
AlightSoulmate:fix-install-invalid-nvm-dir

Conversation

@AlightSoulmate

@AlightSoulmate AlightSoulmate commented Jun 15, 2026

Copy link
Copy Markdown

Refs #1521

The install script now checks invalid $NVM_DIR paths and reports the first
invalid or missing directory.

Tests were added for this case.

@slow6r

This comment was marked as spam.

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a few comments

Comment thread install.sh
Comment on lines +38 to +43
while [ "${INSTALL_DIR}" != "/" ] && [ ! -e "${INSTALL_DIR}" ]; do
INSTALL_DIR="${INSTALL_DIR%/*}"
if [ -z "${INSTALL_DIR}" ]; then
INSTALL_DIR="."
fi
done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two bugs here, both from reducing the path with ${INSTALL_DIR%/*}:

1. Infinite loop on a relative $NVM_DIR. On a slash-less component, ${foo%/*} returns foo unchanged, so this loop never makes progress and install.sh hangs (100% CPU, no output). The INSTALL_DIR="." fallback only fires for absolute single-component paths (where the strip yields ""), so it never rescues the relative case. Reachable in practice: NVM_DIR=mynvm bash install.shnvm_do_installnvm_validate_install_dir → here. The previous code exited cleanly with an error for this input, so this is a regression to a worse failure mode.

2. Wrong base for an absolute path whose first component is missing. ${/a%/*}"" → the . fallback, which is right for relative paths but wrong for absolute ones: nvm_install_dir_base /nonexistent/foo returns . instead of /nonexistent (or /), which surfaces as a ./… path in the error message below.

Suggested fix with dirname (reduces correctly — /a/, foo., a/ba) plus a fixed-point break:

nvm_install_dir_base() {
  local INSTALL_DIR PARENT
  INSTALL_DIR="${1}"
  while [ ! -e "${INSTALL_DIR}" ]; do
    PARENT="$(command dirname "${INSTALL_DIR}")"
    [ "${PARENT}" = "${INSTALL_DIR}" ] && break
    INSTALL_DIR="${PARENT}"
  done
  printf %s "${INSTALL_DIR}"
}

Comment thread install.sh
if [ "${INSTALL_DIR_BASE}" = "/" ]; then
nvm_echo >&2 "Invalid \$NVM_DIR \"${INSTALL_DIR}\": failed to find directory \"/${REMAINING_PATH%%/*}\""
else
nvm_echo >&2 "Invalid \$NVM_DIR \"${INSTALL_DIR}\": failed to find directory \"${INSTALL_DIR_BASE}/${REMAINING_PATH%%/*}\""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Symptom of the nvm_install_dir_base bug above: for an absolute $NVM_DIR whose first component doesn't exist, this emits a relative path. e.g. NVM_DIR=/nonexistentROOT123/foo prints failed to find directory "./nonexistentROOT123", but the missing directory is /nonexistentROOT123. Fixing the base helper resolves this.

echo "${OUTPUT}" | grep -q "Invalid \$NVM_DIR \"${IMPOSSIBLE_DIR}\": failed to find directory \"/dev/null\"" || die "install_nvm_as_script should print invalid NVM_DIR base path, got: ${OUTPUT}"

## nvm_do_install: invalid custom NVM_DIR should report the first missing directory
MISSING_PARENT_DIR="$(pwd)/missing_parent/child"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These cases only exercise bases that already exist - /dev/null (a file) and $(pwd) - so neither nvm_install_dir_base bug is caught, and the suite stays green while the helper is broken. Two cases worth adding:

  • a relative, non-existent NVM_DIR (e.g. NVM_DIR=relnvm) - catches the infinite loop; run it under a timeout/watchdog so a hang fails the test instead of stalling CI.
  • an absolute path whose first component is missing (e.g. NVM_DIR=/nonexistentROOT123/foo) - catches the ./… message bug.

@ljharb
ljharb marked this pull request as draft July 15, 2026 19:29
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