fix: install script bugs (chmod 666, insserv typo, process handling)#35159
fix: install script bugs (chmod 666, insserv typo, process handling)#35159
Conversation
- install.sh: remove chmod 666 /etc/hosts, skip in non-root mode - install.sh: fix insserv $1} stray brace typo - install.sh: replace xargs -r with portable while-read in kill_process() - install_client.sh: fix kill_client() multi-PID handling Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes several issues in the TDengine packaging install scripts, focusing on security (unsafe /etc/hosts permissions), portability (GNU-only xargs -r), and more robust process/service handling during install/upgrade.
Changes:
- Avoid insecure
/etc/hostspermission changes; skip modification in non-root installs and only append when writable. - Replace
xargs -rPID killing withwhile readloops for better cross-platform compatibility. - Fix a SysV init
insservinvocation typo (insserv $1}→insserv $1).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packaging/tools/install.sh | Safer /etc/hosts handling, portable PID-kill fallback, and SysV insserv typo fix |
| packaging/tools/install_client.sh | More consistent handling of multiple PIDs when killing client processes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the installation scripts by refining process termination logic, enhancing the safety of /etc/hosts modifications, and fixing a syntax error in the SysVinit service installation. Specifically, it replaces xargs with while-read loops for killing processes and adds checks for user permissions before attempting to modify system files. A review comment suggests using pgrep -x in install_client.sh to more accurately target specific processes and avoid accidental termination of unrelated services.
- install.sh: add || : to while-read pipeline in kill_process() fallback - install_client.sh: use pgrep -x for exact match, add || true for set -e safety Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Non-root install requires systemd >= 232 for user service support. Shows clear error on older systems (e.g., CentOS 7 with systemd 219). Also adds linger reminder after successful non-root install if not enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packaging/tools/install.sh:737
- In add_newHostname_to_hosts,
grep $1uses an unquoted/regex pattern from user-provided hostname. If the hostname contains regex metacharacters (e.g..) or starts with-, grep can mis-match or treat it as an option. Use fixed-string matching and--(e.g.,grep -F -- "$1") and quote$1when reading from /etc/hosts.
fi
localIp="127.0.0.1"
OLD_IFS="$IFS"
IFS=" "
iphost=$(cat /etc/hosts | grep $1 | awk '{print $1}')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…, fix remove_client non-root - install.sh: Read .install_path in setup_env() to preserve custom install directory on upgrade without -d flag - remove.sh: Add clean_env_file() to remove PATH/LD_LIBRARY_PATH from shell rc file on non-root uninstall - remove_client.sh: Add user_mode detection, .install_path reading, correct paths for non-root, env cleanup. Previously completely broken for non-root. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…stall_main_path The rm -rf was introduced in PR #34172 assuming these paths are always symlinks. When upgrading from older versions that used -d to install data directly in ${installDir}/data (real directory), this would delete user data. These rm calls are entirely unnecessary: - cfg_link_dir and log_link_dir: ln -sf in install_config/install_log already overwrites existing symlinks - data_link_dir: install_data() is NOT called during upgrade, so removing the symlink means it won't be recreated Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tomchon
force-pushed
the
copilot-main-install-script-fixes
branch
from
e6d3fce to
b53dc73
Compare
Reject corrupted .install_path values like '/', '/bin', '/usr', '/etc', '/var' to prevent install_main_path from operating on system directories. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…an_env_file - remove_client.sh: taosgen_name was 'gen' (taosgen) but install_client.sh installs it as 'taosgen' (taostaosgen). Fixed to match installer naming. - remove.sh & remove_client.sh: escape regex metacharacters (especially '.' in .local paths) when building sed patterns for clean_env_file to avoid accidentally removing unrelated export lines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… validation
Replace weak blacklist (/bin, /usr, /etc, /var) with a stronger check:
path must end with '/' (e.g., /taos). This is a natural whitelist
since install always sets installDir="${taosDir}/${PREFIX}", so any
legitimate .install_path will end with /taos.
Rejects /home, /opt, /root, /usr/local, or any other polluted value.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
问题背景
在 3.3.6 非 root 打包 backport 的 review 中，发现 main 分支安装/卸载脚本存在同类问题，可能影响安全性、跨平台兼容性和升级/卸载稳定性。本 PR 将相关修复同步到 main。
主要修复
影响范围
验证建议
关联 PR