Quote variables in destructive commands (#9400 P0)#9401
Quote variables in destructive commands (#9400 P0)#9401iav wants to merge 6 commits intoarmbian:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 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).
| display_alert "Creatng abl kernel boot recovery image with dtb ${ABL_DTB_LIST[0]}" "${EXTENSION}" "info" | ||
| 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/"${dtb_name}.dtb" > "${DESTIMG}/Image.gz-${dtb_name}" | ||
| /usr/bin/mkbootimg \ | ||
| --kernel ${DESTIMG}/Image.gz-${ABL_DTB_LIST[0]} \ | ||
| --ramdisk ${new_rootfs_image_mount_dir}/boot/initrd.img-*-* \ | ||
| --kernel "${DESTIMG}/Image.gz-${ABL_DTB_LIST[0]}" \ | ||
| --ramdisk "${new_rootfs_image_mount_dir}"/boot/initrd.img-*-* \ | ||
| --base 0x0 \ | ||
| --second_offset 0x00f00000 \ | ||
| --kernel_offset 0x8000 \ | ||
| --ramdisk_offset 0x1000000 \ | ||
| --tags_offset 0x100 \ | ||
| --pagesize 4096 \ | ||
| -o ${DESTIMG}/${version}.boot_recovery.img | ||
| -o "${DESTIMG}/${version}.boot_recovery.img" |
There was a problem hiding this comment.
Pre-existing bug: ${dtb_name} on line 59 should be ${ABL_DTB_LIST[0]}.
After the for loop (lines 43–57), $dtb_name holds the last element of ABL_DTB_LIST, not the first. Line 58's display alert and line 61's --kernel argument both correctly reference ${ABL_DTB_LIST[0]}, but line 59's cat still uses ${dtb_name}, producing an inconsistent DTB concatenation for the recovery image.
Not introduced by this PR, but worth fixing alongside.
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]}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| display_alert "Creatng abl kernel boot recovery image with dtb ${ABL_DTB_LIST[0]}" "${EXTENSION}" "info" | |
| 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/"${dtb_name}.dtb" > "${DESTIMG}/Image.gz-${dtb_name}" | |
| /usr/bin/mkbootimg \ | |
| --kernel ${DESTIMG}/Image.gz-${ABL_DTB_LIST[0]} \ | |
| --ramdisk ${new_rootfs_image_mount_dir}/boot/initrd.img-*-* \ | |
| --kernel "${DESTIMG}/Image.gz-${ABL_DTB_LIST[0]}" \ | |
| --ramdisk "${new_rootfs_image_mount_dir}"/boot/initrd.img-*-* \ | |
| --base 0x0 \ | |
| --second_offset 0x00f00000 \ | |
| --kernel_offset 0x8000 \ | |
| --ramdisk_offset 0x1000000 \ | |
| --tags_offset 0x100 \ | |
| --pagesize 4096 \ | |
| -o ${DESTIMG}/${version}.boot_recovery.img | |
| -o "${DESTIMG}/${version}.boot_recovery.img" | |
| display_alert "Creatng abl kernel boot recovery image with dtb ${ABL_DTB_LIST[0]}" "${EXTENSION}" "info" | |
| 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]}" | |
| /usr/bin/mkbootimg \ | |
| --kernel "${DESTIMG}/Image.gz-${ABL_DTB_LIST[0]}" \ | |
| --ramdisk "${new_rootfs_image_mount_dir}"/boot/initrd.img-*-* \ | |
| --base 0x0 \ | |
| --second_offset 0x00f00000 \ | |
| --kernel_offset 0x8000 \ | |
| --ramdisk_offset 0x1000000 \ | |
| --tags_offset 0x100 \ | |
| --pagesize 4096 \ | |
| -o "${DESTIMG}/${version}.boot_recovery.img" |
🤖 Prompt for AI Agents
In `@extensions/image-output-abl.sh` around lines 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).
There was a problem hiding this comment.
Good catch — this is indeed a pre-existing bug introduced in commit 9358fe7 by @amazingfate (Jan 2025, #7663 "image-output-abl: build boot recovery image").
@amazingfate Could you confirm: should line 59 use ${ABL_DTB_LIST[0]} instead of ${dtb_name} (which retains the last loop iteration value)? The display_alert on line 58 and --kernel on line 61 both reference ${ABL_DTB_LIST[0]}, so the cat on line 59 looks inconsistent.
I'll add a fix in this PR if you confirm.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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
from
e681557 to
10b5ba5
Compare
…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>
iav
force-pushed
the
fix/p0-quote-variables
branch
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
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>
iav
force-pushed
the
fix/p0-quote-variables
branch
from
2f75e1c to
39f7f37
Compare
2 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