Skip to content

Commit 9ed9496

Browse files
committed
review(bklibcvenv): address Copilot's 5 inline comments on #348
All 5 points were legit: 1. **shift on empty args** (libexec/bklibcvenv:39) — with set -eo pipefail, `CMD=$1; shift` exited before the usage-error branch could fire when called with zero args. Now: check `$#` first. 2. **set -x unconditional** (line 73) — floods CI output. Gated behind BKLIBCVENV_DEBUG env var. 3. **chmod 775** (line 94) — group-writable on packaged artefacts is a bad-practice flag for security/packaging audits. Changed to 755. 4. **POSIX `--` in command -v / cd** (template line 12, 15) — not specified by POSIX for these built-ins; some old /bin/sh implementations reject. Dropped `--`; in our context $0 is never user-controlled so the defense was moot. 5. **sed-replacement metacharacter unsafety** (line 93) — values containing `&`, `\`, or the `|` delimiter would corrupt sed output. Defense in depth: added a `sed_escape()` helper and applied to all four interpolated values before substitution. Values in question (LDSO, LIBDIR_NAME, LIBC_NAME, dir) are all auto-detected or recipe-passed and won't realistically contain sed metachars, but Copilot's right that this should not be a latent footgun.
1 parent b4fab13 commit 9ed9496

2 files changed

Lines changed: 38 additions & 11 deletions

File tree

libexec/bklibcvenv

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,28 @@
3535

3636
set -eo pipefail
3737

38-
CMD=$1; shift
38+
if [ $# -lt 1 ]; then
39+
echo "bklibcvenv: missing subcommand" >&2
40+
echo "usage: $0 seal <prefix> <libdir-name>" >&2
41+
exit 64
42+
fi
43+
44+
CMD=$1
45+
shift
46+
47+
# Optional debug tracing — opt-in via env to avoid log noise.
48+
[ -n "${BKLIBCVENV_DEBUG:-}" ] && set -x
49+
50+
# sed-replacement-safe escape. Backslashes, ampersands, and the
51+
# `|` delimiter need to be quoted in the replacement-half of an
52+
# `s|…|…|g`. Without escaping a value like `foo&bar` would be
53+
# replaced by sed's match-back-reference; `foo\1bar` similarly
54+
# breaks. In practice our values are tightly controlled (LDSO,
55+
# LIBDIR_NAME etc. come from arch/version, never user input), so
56+
# this is defense in depth.
57+
sed_escape() {
58+
printf '%s\n' "$1" | sed -e 's/[\\&|]/\\&/g'
59+
}
3960

4061
case "$CMD" in
4162
seal)
@@ -63,14 +84,17 @@ case "$CMD" in
6384
LIBC_NAME=${LIBDIR_NAME%%-*}
6485

6586
# Locate the wrapper template (share/brewkit/libcvenv-wrapper.sh).
66-
SELF_DIR=$(CDPATH= cd -- "$(dirname "$0")" && pwd)
87+
SELF_DIR=$(CDPATH= cd "$(dirname "$0")" && pwd)
6788
TEMPLATE="$SELF_DIR/../share/brewkit/libcvenv-wrapper.sh"
6889
if [ ! -f "$TEMPLATE" ]; then
6990
echo "bklibcvenv: wrapper template not found at $TEMPLATE" >&2
7091
exit 2
7192
fi
7293

73-
set -x
94+
# Escape sed-replacement values once up-front.
95+
LDSO_ESC=$(sed_escape "$LDSO")
96+
LIBDIR_ESC=$(sed_escape "$LIBDIR_NAME")
97+
LIBC_ESC=$(sed_escape "$LIBC_NAME")
7498

7599
for dir in bin sbin; do
76100
[ -d "$PREFIX/$dir" ] || continue
@@ -85,13 +109,14 @@ case "$CMD" in
85109
name=$(basename "$f")
86110
mv "$f" "$PREFIX/libexec/$LIBC_NAME-$dir/"
87111

112+
DIR_ESC=$(sed_escape "$dir")
88113
sed \
89-
-e "s|@LDSO@|$LDSO|g" \
90-
-e "s|@LIBDIR@|$LIBDIR_NAME|g" \
91-
-e "s|@LIBC_NAME@|$LIBC_NAME|g" \
92-
-e "s|@DIR@|$dir|g" \
114+
-e "s|@LDSO@|$LDSO_ESC|g" \
115+
-e "s|@LIBDIR@|$LIBDIR_ESC|g" \
116+
-e "s|@LIBC_NAME@|$LIBC_ESC|g" \
117+
-e "s|@DIR@|$DIR_ESC|g" \
93118
"$TEMPLATE" > "$f"
94-
chmod 775 "$f"
119+
chmod 755 "$f"
95120

96121
echo "wrapped $f"
97122
done

share/brewkit/libcvenv-wrapper.sh

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#!/bin/sh
2-
# Template for bklibcvenv-generated wrappers.
2+
# Template for bklibcvenv-generated wrappers. Pure POSIX sh — no
3+
# `--` end-of-options markers (POSIX doesn't specify them for `cd`
4+
# or `command -v`; some old /bin/sh implementations reject them).
35
#
46
# Replaced by `bklibcvenv seal`:
57
# @LDSO@ e.g. ld-linux-x86-64.so.2
@@ -9,10 +11,10 @@
911

1012
case "$0" in
1113
*/*) bindir=${0%/*} ;;
12-
*) bindir=$(command -v -- "$0"); bindir=${bindir%/*} ;;
14+
*) bindir=$(command -v "$0"); bindir=${bindir%/*} ;;
1315
esac
1416

15-
prefix=$(CDPATH= cd -- "$bindir/.." && pwd)
17+
prefix=$(CDPATH= cd "$bindir/.." && pwd)
1618
libdir="$prefix/lib/@LIBDIR@"
1719

1820
exec "$libdir/@LDSO@" --library-path "$libdir" "$prefix/libexec/@LIBC_NAME@-@DIR@/$(basename "$0")" "$@"

0 commit comments

Comments
 (0)