imbajin commented on code in PR #3253:
URL: https://github.com/apache/hugegraph/pull/3253#discussion_r4359870599
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/restart-hugegraph-store.sh:
##########
@@ -28,5 +28,83 @@ function abs_path() {
}
BIN=$(abs_path)
-. "$BIN"/stop-hugegraph-store.sh
-. "$BIN"/start-hugegraph-store.sh
+TOP="$(cd "$BIN"/../ && pwd)"
+
+DAEMON_ARG=""
+CONF_ARG=""
+GC_ARG=""
+PID_ARG=""
+USER_ARG=""
+LOGS_ARG=""
+PLUGINS_ARG=""
+TELEMETRY_ARG=""
+
+while getopts "d:c:g:i:j:l:o:y:" arg; do
+ case ${arg} in
+ d) DAEMON_ARG="$OPTARG" ;;
+ c) CONF_ARG="$OPTARG" ;;
+ g) GC_ARG="$OPTARG" ;;
+ i) PID_ARG="$OPTARG" ;;
+ j) USER_ARG="$OPTARG" ;;
+ l) LOGS_ARG="$OPTARG" ;;
+ o) PLUGINS_ARG="$OPTARG" ;;
+ y) TELEMETRY_ARG="$OPTARG" ;;
+ ?) echo "USAGE: $0 [-d true|false] [-c conf_dir] [-g g1] [-i pid_file]
[-j opts] [-l logs_dir] [-o plugins_dir] [-y true|false]" && exit 1 ;;
+ esac
+done
+
+. "$BIN"/util.sh
+
+# -c/-l/-o are validated only by start-hugegraph-store.sh, which runs *after*
+# the stop below. Check them here so an unusable override is rejected while the
+# Store is still up, instead of stopping it and then failing to start it again.
+# -i needs no check here: stop-hugegraph-store.sh validates it first, before it
+# touches the running process.
+reject_unsafe_path "$CONF_ARG" "-c conf dir" crlf || exit 1
+reject_unsafe_path "$LOGS_ARG" "-l logs dir" crlf || exit 1
+# The plugins dir is joined into ':'-separated classpath/agent settings
+reject_unsafe_path "$PLUGINS_ARG" "-o plugins dir" classpath || exit 1
+
+# The checks above only reject unsafe characters; they don't prove -c/-l/-o
+# are actually usable. Resolve them exactly as start-hugegraph-store.sh will
+# and run its writability/readability guards here too, so a bad
+# override is caught while the current Store is still up and serving traffic
+RESTART_CONF_OVERRIDE="$(canonicalize_dir "$CONF_ARG" nocreate)" || exit 1
+RESTART_LOGS_OVERRIDE="$(canonicalize_dir "$LOGS_ARG")" || exit 1
+RESTART_PLUGINS_OVERRIDE="$(canonicalize_dir "$PLUGINS_ARG")" || exit 1
+RESTART_PID_FILE_OVERRIDE="$(canonicalize_file "$PID_ARG")" || exit 1
+
+RESTART_CONF="${RESTART_CONF_OVERRIDE:-$TOP/conf}"
+RESTART_LOGS="${RESTART_LOGS_OVERRIDE:-$TOP/logs}"
Review Comment:
⚠️ Important: A no-argument restart ignores the inherited overrides during
this preflight. Store exports
CONF_OVERRIDE/LOGS_OVERRIDE/PLUGINS_OVERRIDE/PID_FILE_OVERRIDE, and
PdConfigureListener.restart() launches this script without arguments, but the
*_ARG variables start empty. Lines 77–84 therefore check the packaged
logs/plugins/bin directories rather than the external directories the child
launchers will use. A Store running from a read-only installation with valid
external paths cannot perform its PD-triggered restart. Please use inherited
overrides as defaults before CLI parsing/canonicalization, and cover a
no-argument restart with inherited external paths.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:
##########
@@ -39,30 +39,60 @@ function abs_path() {
BIN=$(abs_path)
TOP="$(cd "$BIN"/../ && pwd)"
-CONF="$TOP/conf"
-LOGS="$TOP/logs"
SCRIPTS="$TOP/scripts"
-PID_FILE="$BIN/pid"
. "$BIN"/util.sh
# Note: keep ':' in the end of the string to indicate the option needs a value
-while getopts "d:g:m:p:s:j:t:y:" arg; do
- case ${arg} in
- d) DAEMON="$OPTARG" ;;
- g) GC_OPTION="$OPTARG" ;;
- m) OPEN_MONITOR="$OPTARG" ;;
- p) PRELOAD="$OPTARG" ;;
- s) OPEN_SECURITY_CHECK="$OPTARG" ;;
- j) USER_OPTION="$OPTARG" ;;
- t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
- # Telemetry is used to collect metrics, traces and logs
- y) OPEN_TELEMETRY="$OPTARG" ;;
- # Note: update usage info when the params changed
- ?) exit_with_usage_help ;;
- esac
+while getopts "c:d:g:i:j:l:m:o:p:s:t:y:" arg; do
+ case ${arg} in
+ c) reject_unsafe_path "$OPTARG" "-c conf dir" crlf || exit 1
+ CONF_OVERRIDE="$OPTARG" ;;
+ d) DAEMON="$OPTARG" ;;
+ g) GC_OPTION="$OPTARG" ;;
+ i) reject_unsafe_path "$OPTARG" "-i pid file" crlf || exit 1
+ PID_FILE_OVERRIDE="$OPTARG" ;;
+ j) USER_OPTION="$OPTARG" ;;
+ l) reject_unsafe_path "$OPTARG" "-l logs dir" crlf || exit 1
+ LOGS_OVERRIDE="$OPTARG" ;;
+ m) OPEN_MONITOR="$OPTARG" ;;
+ o) reject_unsafe_path "$OPTARG" "-o plugins dir" classpath || exit 1
+ PLUGINS_OVERRIDE="$OPTARG" ;;
+ p) PRELOAD="$OPTARG" ;;
+ s) OPEN_SECURITY_CHECK="$OPTARG" ;;
+ t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
+ # Telemetry is used to collect metrics, traces and logs
+ y) OPEN_TELEMETRY="$OPTARG" ;;
+ # Note: update usage info when the params changed
+ ?) exit_with_usage_help ;;
+ esac
done
+# Canonicalize relative path overrides to absolute paths.
+CONF_OVERRIDE="$(canonicalize_dir "$CONF_OVERRIDE" nocreate)" || exit 1
+LOGS_OVERRIDE="$(canonicalize_dir "$LOGS_OVERRIDE")" || exit 1
+PLUGINS_OVERRIDE="$(canonicalize_dir "$PLUGINS_OVERRIDE")" || exit 1
Review Comment:
⚠️ Important: The supported environment-variable entry point bypasses the
raw-value guards, which run only inside getopts. For example,
PLUGINS_OVERRIDE=/data/plugins:external reaches canonicalize_dir without the
classpath validation applied to -o, then hugegraph-server.sh joins discovered
JARs into a colon-delimited classpath and splits those paths into invalid
entries. Store has the same gap, and inherited CR/LF values also bypass the
pre-canonicalization checks. Please validate all four effective raw override
variables after option parsing and before canonicalization, as the PD launcher
already does, and cover environment inputs.
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -152,55 +187,78 @@ else
fi
# check jdk version
-JAVA_VERSION=$($JAVA -version 2>&1 |
+JAVA_VERSION=$("$JAVA" -version 2>&1 |
awk -F'"' '/^(java|openjdk) version "/ {print $2; exit}' |
sed 's/^1\.//' | cut -d'.' -f1)
JAVA_VERSION="${JAVA_VERSION%%[!0-9]*}"
if [[ -z $JAVA_VERSION || $JAVA_VERSION -lt $EXPECT_JDK_VERSION ]]; then
- echo "Please make sure that the JDK is installed and the version >=
$EXPECT_JDK_VERSION" >> ${OUTPUT}
+ echo "Please make sure that the JDK is installed and the version >=
$EXPECT_JDK_VERSION" >> "${OUTPUT}"
exit 1
fi
-# Set Java options
-if [ "$JAVA_OPTIONS" = "" ]; then
+# Set Java options.
+# Script-generated options are kept in arrays so that paths containing spaces
+# (overridden CONF/LOGS/PLUGINS) stay a single argument when passed to java.
+# User-supplied strings ($JAVA_OPTIONS / $USER_OPTION) keep the historical
+# word-splitting contract.
+JAVA_OPTIONS_ENV="${JAVA_OPTIONS}"
+JAVA_OPTIONS=()
Review Comment:
⚠️ Important: Converting JAVA_OPTIONS from an exported scalar into a Bash
array removes it from the environment inherited by Java. A Bash probe with
exported JAVA_OPTIONS='-Xmx8g -Dexample=required' and this assignment/parsing
sequence confirms that a child env no longer contains JAVA_OPTIONS.
PdConfigureListener.restart() starts restart-hugegraph-store.sh without
arguments, so the next Store launch loses those heap/system settings and falls
back to defaults. Please retain the exported scalar and build the launch
arguments in a separate array, with a regression that checks the environment
inherited by the restart child.
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -152,55 +187,78 @@ else
fi
# check jdk version
-JAVA_VERSION=$($JAVA -version 2>&1 |
+JAVA_VERSION=$("$JAVA" -version 2>&1 |
awk -F'"' '/^(java|openjdk) version "/ {print $2; exit}' |
sed 's/^1\.//' | cut -d'.' -f1)
JAVA_VERSION="${JAVA_VERSION%%[!0-9]*}"
if [[ -z $JAVA_VERSION || $JAVA_VERSION -lt $EXPECT_JDK_VERSION ]]; then
- echo "Please make sure that the JDK is installed and the version >=
$EXPECT_JDK_VERSION" >> ${OUTPUT}
+ echo "Please make sure that the JDK is installed and the version >=
$EXPECT_JDK_VERSION" >> "${OUTPUT}"
exit 1
fi
-# Set Java options
-if [ "$JAVA_OPTIONS" = "" ]; then
+# Set Java options.
+# Script-generated options are kept in arrays so that paths containing spaces
+# (overridden CONF/LOGS/PLUGINS) stay a single argument when passed to java.
+# User-supplied strings ($JAVA_OPTIONS / $USER_OPTION) keep the historical
+# word-splitting contract.
+JAVA_OPTIONS_ENV="${JAVA_OPTIONS}"
+JAVA_OPTIONS=()
+if [ "$JAVA_OPTIONS_ENV" = "" ]; then
XMX=$(calc_xmx $MIN_MEM $MAX_MEM)
if [ $? -ne 0 ]; then
echo "Failed to start HugeGraphStoreServer, requires at least
${MIN_MEM}m free memory" \
- >> ${OUTPUT}
+ >> "${OUTPUT}"
exit 1
fi
- JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m -XX:MetaspaceSize=256M
-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=${LOGS} ${USER_OPTION} "
- # JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m
-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=${LOGS} ${USER_OPTION}"
+ JAVA_OPTIONS=("-Xms${MIN_MEM}m" "-Xmx${XMX}m" "-XX:MetaspaceSize=256M"
+ "-XX:+HeapDumpOnOutOfMemoryError" "-XX:HeapDumpPath=${LOGS}")
+
+ # Split user-supplied options on whitespace (legacy behaviour).
+ # Use NUL (not newline) as the read delimiter so multi-line values
+ # (e.g. Helm javaOpts/Compose `|` blocks) are not silently truncated
+ # at the first line.
+ USER_OPTIONS=()
+ read -r -d '' -a USER_OPTIONS <<< "${USER_OPTION}"$'\0' || true
+ JAVA_OPTIONS+=(${USER_OPTIONS[@]+"${USER_OPTIONS[@]}"})
# Rolling out detailed GC logs
- JAVA_OPTIONS="${JAVA_OPTIONS}
-Xlog:gc=info:file=./logs/gc.log:time,uptime,level,tags:filecount=3,filesize=100m"
+
JAVA_OPTIONS+=("-Xlog:gc=info:file=${LOGS}/gc.log:time,uptime,level,tags:filecount=3,filesize=100m")
Review Comment:
⚠️ Important: -l accepts ':' in a directory name, but this value is also
embedded in the JVM unified-logging grammar, where ':' separates fields.
Quoting the Bash array element does not escape that grammar. Running java
'-Xlog:gc=info:file=/tmp/hg-review:logs/gc.log:time,uptime,level,tags:filecount=3,filesize=100m'
-version exits 1 with Invalid decorator 'logs/gc.log' and Could not create the
Java Virtual Machine. Thus a valid external log directory containing ':'
prevents Store startup with default JVM options. Please encode the filename for
unified logging or reject unsupported delimiters before launch, and add a
real-JVM regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
