Quote variables in destructive commands (#9400 P0)#9401
Quote variables in destructive commands (#9400 P0)#9401iav wants to merge 7 commits intoarmbian:mainfrom
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMultiple shell scripts and a partitioning library were updated to consistently quote variable expansions and file paths (e.g., "${SDCARD}", "${config_src}", "${kernel_work_dir}"). Changes are syntactic only and do not alter control flow, interfaces, or runtime behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@extensions/image-output-abl.sh`:
- Around line 58-69: The cat command is using the loop variable dtb_name (which
contains the last element) causing an inconsistent DTB to be concatenated;
update the cat invocation to use the first ABL DTB reference ${ABL_DTB_LIST[0]}
instead of ${dtb_name} so it matches the display_alert and the --kernel argument
passed to mkbootimg (ensure the temporary file name and the --kernel value both
derive from ${ABL_DTB_LIST[0]} to keep them consistent with DESTIMG,
new_rootfs_image_mount_dir, and version).
🧹 Nitpick comments (1)
lib/functions/image/partitioning.sh (1)
352-387: Unquoted variables in heredoc extension hooks still carry word-splitting risk.The heredoc blocks passed to
call_extension_methodat lines 352–362 and 371–387 contain unquoted variables in destructive commands (e.g.,btrfs subvolume create $MOUNT/@home,echo ... >> $SDCARD/etc/fstab). The PR description notes these are intentionally excluded, but since this code is executed (not just documentation), it has the same word-splitting risk the rest of the PR addresses.Consider a follow-up to quote these as well, or document why they're excluded (e.g., if the heredoc quoting context makes it non-trivial).
iav
force-pushed
the
fix/p0-quote-variables
branch
from
d858aa8 to
2874385
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/image/partitioning.sh (1)
350-350:⚠️ Potential issue | 🟡 MinorUnquoted
$MOUNT/$btrfs_root_subvolumeinside string argument.Line 350 passes an unquoted path as a string to
run_host_command_logged. Since this function executes viabash -c "$*", the inner shell will word-split$MOUNT/$btrfs_root_subvolumeif either variable contains spaces. The adjacent line 346 was already fixed — this one appears to have been missed.Proposed fix
- run_host_command_logged "btrfs subvolume set-default $MOUNT/$btrfs_root_subvolume" + run_host_command_logged "btrfs subvolume set-default '$MOUNT/$btrfs_root_subvolume'"Note: Since
run_host_command_loggedconcatenates args intobash -c "$*", embedding single quotes inside the string protects the path in the inner shell. Alternatively, pass as separate arguments like line 346 does:run_host_command_logged btrfs subvolume set-default "$MOUNT/$btrfs_root_subvolume".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/image/partitioning.sh` at line 350, The call to run_host_command_logged uses an unquoted expansion run_host_command_logged "btrfs subvolume set-default $MOUNT/$btrfs_root_subvolume" which allows word-splitting if $MOUNT or $btrfs_root_subvolume contain spaces; change the invocation to pass the command and the path as separate arguments so the path is quoted by the outer shell (e.g., call run_host_command_logged btrfs subvolume set-default "$MOUNT/$btrfs_root_subvolume") or alternatively embed the path in single quotes within the string, ensuring the variables are properly quoted to prevent splitting; update the invocation that contains run_host_command_logged and the variables MOUNT and btrfs_root_subvolume accordingly.
🧹 Nitpick comments (1)
lib/functions/image/partitioning.sh (1)
434-434: Minor: inconsistent quoting style for${SDCARD}.Line 434 quotes
"${SDCARD}/etc/fstab"— good. But line 433 (unchanged) uses$SDCARD/etc/fstabunquoted in thedisplay_alertargument. Not a safety issue sincedisplay_alertisn't destructive, just noting the inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/image/partitioning.sh` at line 434, The display_alert call uses an unquoted $SDCARD while later code appends to "${SDCARD}/etc/fstab", causing inconsistent quoting; update the display_alert invocation to use "${SDCARD}" (i.e., change $SDCARD/... to "${SDCARD}/...") so the SDCARD variable is quoted consistently across the script (refer to the display_alert call and the "${SDCARD}/etc/fstab" append).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/functions/image/partitioning.sh`:
- Line 350: The call to run_host_command_logged uses an unquoted expansion
run_host_command_logged "btrfs subvolume set-default
$MOUNT/$btrfs_root_subvolume" which allows word-splitting if $MOUNT or
$btrfs_root_subvolume contain spaces; change the invocation to pass the command
and the path as separate arguments so the path is quoted by the outer shell
(e.g., call run_host_command_logged btrfs subvolume set-default
"$MOUNT/$btrfs_root_subvolume") or alternatively embed the path in single quotes
within the string, ensuring the variables are properly quoted to prevent
splitting; update the invocation that contains run_host_command_logged and the
variables MOUNT and btrfs_root_subvolume accordingly.
---
Nitpick comments:
In `@lib/functions/image/partitioning.sh`:
- Line 434: The display_alert call uses an unquoted $SDCARD while later code
appends to "${SDCARD}/etc/fstab", causing inconsistent quoting; update the
display_alert invocation to use "${SDCARD}" (i.e., change $SDCARD/... to
"${SDCARD}/...") so the SDCARD variable is quoted consistently across the script
(refer to the display_alert call and the "${SDCARD}/etc/fstab" append).
2874385 to
e681557
Compare
iav
force-pushed
the
fix/p0-quote-variables
branch
2 times, most recently
from
10b5ba5 to
2f75e1c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
extensions/image-output-abl.sh (1)
59-59:⚠️ Potential issue | 🟡 MinorPre-existing bug:
${dtb_name}on the recovery imagecatuses the last loop value, not${ABL_DTB_LIST[0]}.Line 58's
display_alertand line 61's--kernelboth reference${ABL_DTB_LIST[0]}, but thecaton line 59 still reads${dtb_name}— which retains whatever value theforloop left it with (the last element ofABL_DTB_LIST). This means the recovery kernel image is silently built from the wrong DTB when the list has more than one entry.🐛 Proposed fix
- cat "${DESTIMG}/Image.gz" "${new_rootfs_image_mount_dir}"/usr/lib/linux-image-*/qcom/"${dtb_name}.dtb" > "${DESTIMG}/Image.gz-${dtb_name}" + cat "${DESTIMG}/Image.gz" "${new_rootfs_image_mount_dir}"/usr/lib/linux-image-*/qcom/"${ABL_DTB_LIST[0]}.dtb" > "${DESTIMG}/Image.gz-${ABL_DTB_LIST[0]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/image-output-abl.sh` at line 59, The cat command that builds the recovery kernel image is using the loop variable ${dtb_name} (which ends up as the last element) instead of the first DTB entry referenced elsewhere; update the cat invocation that writes "${DESTIMG}/Image.gz-${dtb_name}" to use the first list element "${ABL_DTB_LIST[0]}" so it matches the display_alert and --kernel usage and ensures the recovery image is created from ABL_DTB_LIST[0]; confirm the replacement in the command that reads from "${new_rootfs_image_mount_dir}/usr/lib/linux-image-*/qcom/" and that filename construction remains correct.
🧹 Nitpick comments (1)
lib/functions/image/partitioning.sh (1)
323-323: Consider adding ashellcheck disable=SC2086comment for${fscreateopt}to match the pattern established for${mkopts}at line 404.
${fscreateopt}must remain unquoted so-o compress-force=…word-splits into two arguments, and it correctly expands to zero words when unset. The same intentional-unquoting rationale applies here as at line 404, but no annotation was added at this site (or at line 366).♻️ Proposed fix
+ # shellcheck disable=SC2086 # fscreateopt must word-split into "-o" and "compress-force=..." when set run_host_command_logged mount ${fscreateopt} "$rootdevice" "$MOUNT"/And similarly at line 366:
+ # shellcheck disable=SC2086 # fscreateopt must word-split into "-o" and "compress-force=..." when set run_host_command_logged mount -odefaults,${mountopts[$ROOTFS_TYPE]} ${fscreateopt} "$rootdevice" "$MOUNT"/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/image/partitioning.sh` at line 323, The mount call uses an intentional unquoted variable ${fscreateopt} (in the run_host_command_logged mount invocation) and needs a shellcheck exemption like the existing ${mkopts} usage; add a comment "# shellcheck disable=SC2086" immediately above the run_host_command_logged mount "${fscreateopt}" expansion to document the deliberate word-splitting/empty expansion behavior, and add the same exemption at the other site where ${fscreateopt} is intentionally unquoted (the earlier run_host_command_logged invocation that mirrors the mkopts pattern).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@extensions/image-output-abl.sh`:
- Line 59: The cat command that builds the recovery kernel image is using the
loop variable ${dtb_name} (which ends up as the last element) instead of the
first DTB entry referenced elsewhere; update the cat invocation that writes
"${DESTIMG}/Image.gz-${dtb_name}" to use the first list element
"${ABL_DTB_LIST[0]}" so it matches the display_alert and --kernel usage and
ensures the recovery image is created from ABL_DTB_LIST[0]; confirm the
replacement in the command that reads from
"${new_rootfs_image_mount_dir}/usr/lib/linux-image-*/qcom/" and that filename
construction remains correct.
---
Nitpick comments:
In `@lib/functions/image/partitioning.sh`:
- Line 323: The mount call uses an intentional unquoted variable ${fscreateopt}
(in the run_host_command_logged mount invocation) and needs a shellcheck
exemption like the existing ${mkopts} usage; add a comment "# shellcheck
disable=SC2086" immediately above the run_host_command_logged mount
"${fscreateopt}" expansion to document the deliberate word-splitting/empty
expansion behavior, and add the same exemption at the other site where
${fscreateopt} is intentionally unquoted (the earlier run_host_command_logged
invocation that mirrors the mkopts pattern).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
extensions/allwinner-kernel-bump.shextensions/cloud-init/cloud-init.shextensions/image-output-abl.shextensions/lvm.shextensions/uwe5622-allwinner.shlib/functions/image/partitioning.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- extensions/cloud-init/cloud-init.sh
- extensions/lvm.sh
- extensions/allwinner-kernel-bump.sh
iav
force-pushed
the
fix/p0-quote-variables
branch
from
2f75e1c to
39f7f37
Compare
…age build
The cat command creating Image.gz-${dtb_name} (where dtb_name holds the last
loop value) was unnecessary: Image.gz-${ABL_DTB_LIST[0]} is already created
by the for loop, and mkbootimg for the recovery image uses exactly that file.
Fix per armbian#9401 (comment)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…armbian#9400 P0) Quote all variable expansions passed to rm, mount, umount, losetup, mkfs, sed -i, cp, truncate, blkid, gzip, cat, e2fsck, resize2fs, and source to prevent word splitting. Replace `[ ! -z ]` with `[[ -n ]]`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
armbian#9400 P0) Quote $SDCARD in chroot calls and $destination, $SRC, ${ARCH} in cp, mkdir, and install commands to prevent word splitting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ian#9400 P0) Quote ${config_src}, $config_dst, ${defaults_src}, $defaults_dst in cp commands and ${SDCARD} in rm -f commands to prevent word splitting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…P0)
Quote ${SDCARD}.raw, ${rootdevice}, ${LVM_VG_NAME}, ${volsize} in
parted, pvcreate, vgcreate, lvcreate, e2label, blkid, and vgchange
commands to prevent word splitting.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mands (armbian#9400 P0) Quote ${patch_dir_base}, ${patch_dir_megous}, ${patch_dir_tmp}, ${kernel_work_dir}, ${bundle_file}, ${PREV_KERNEL_PATCH_DIR} in rm, cp, mv, mkdir, git, grep, sed -i, and cat commands. Fix unquoted array expansion: ${megous_trees[@]} → "${megous_trees[@]}". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P0) Quote $SDCARD, $MOUNT, $rootdevice, ${LOOP}p${rootpart}, $bootscript_dst and other variables in rm, mount, umount, mkdir, sed -i, echo >>, mkfs, blkid, grep, and cat commands to prevent word splitting. Replace `[ -z ]` with `[[ -z ]]` on line guarding boot script output. Heredoc blocks inside call_extension_method are left unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…age build
The cat command creating Image.gz-${dtb_name} (where dtb_name holds the last
loop value) was unnecessary: Image.gz-${ABL_DTB_LIST[0]} is already created
by the for loop, and mkbootimg for the recovery image uses exactly that file.
Fix per armbian#9401 (comment)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
iav
force-pushed
the
fix/p0-quote-variables
branch
from
b09e995 to
6940368
Compare
|
rebased |
|
✅ This PR has been reviewed and approved — all set for merge! |
4 participants
Summary
rm,mv,cp,mount,umount,losetup,sed -i,mkdir,mkfs,echo >>,blkid,chroot, etc.) to prevent word splitting — the highest-priority safety fix from Proposal: Improve Bash syntax safety across the codebase #9400[ ! -z ]/[ -z ]with[[ -n ]]/[[ -z ]]where they guard destructive operations${megous_trees[@]}→"${megous_trees[@]}"Files changed (one commit per file)
extensions/image-output-abl.shextensions/uwe5622-allwinner.shextensions/cloud-init/cloud-init.shextensions/lvm.shextensions/allwinner-kernel-bump.shlib/functions/image/partitioning.shHeredoc documentation blocks inside
call_extension_methodare intentionally left unchanged.Test plan
shellcheckpasses with no new warnings🤖 Generated with Claude Code
Summary by CodeRabbit