04/29/2012 06:46 PM, Ian Geiser:
> Greetings, it seems that the regexp used to see if a device is
> blacklisted fails on busybox for me. I have attached a patch to
> change the regexp to use word boundaries instead of line boundaries.
(You mean the opposite, right? Your patch makes the regex use line
boundaries whereas the original coude used word boundaries.)
After reading GNU grep's man page (and assuming that busybox's grep does
the same thing) I must say that the bug you found (and I introduced, see
see commit f885467) and tried to fix isn't surprising. Let me quote:
The symbols \< and \> respectively match the empty string at
the beginning and end of a word.
In "\<${fulldevname}\>" (i.e. the old pattern) the first character of
${fulldevname} will always be "/" since a device is an absolute path.
Word characters are [[:alnum:]_], so "/" is a non-word character, so
"\<" isn't in the beginning of a word and the regex will not match.
> I hope this patch proves useful and is merged into head.
AFAICT your patch breaks storage_devices() in two ways:
1. Send in 2+ devices to the white list an nothing is white listed.
This should completely break the persistence-media=removable(-usb)
option when 2+ removable (USB) devices are present.
2. Send in 2+ devices to the black list and nothing is black listed.
This is (coincidentally) not a big issue in the current state of
live-boot's code since it's only ever used with one black listed
device at a time. OTOH this subtlety could very well cause
unintended bugs in the future since one expects the black list to
work like the white list (which *must* accept more than 2+ devices).
I think the Right Thing(TM) to do is to implement a robust is_in_list()
function instead of using ad-hoc grepping, both here and and at other
places in the code. See the attached patch. The is_in_list() function
therein:
* interprets white-spaces and newlines as separators; all other
chacaters form "words", so there should be no more surprises.
* doesn't depend on quoting input to form "lists".
* doesn't use any fancy grep functionality (no -w, no \< or \>), just
basic regex stuff that ought to be correctly implemented in busybox.
* makes the code much more readable.
I chose to email a patch instead of pushing directly since I'm not sure
if the policy here would be to revert/reset the offending commit since
it's already pushed.
Cheers!
From 141ca2c5856c56437bb77180fee7409c6c7a1f72 Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@boum.org>
Date: Thu, 3 May 2012 13:54:00 +0200
Subject: [PATCH] Implement and use is_in_list() for list membership checking.
This also fixes a bug introduced in the previous commit which made
storage_devices() white-list break for 2+ devices, which in turn
breaks the persistence-media=removable(-usb) boot parameter.
---
scripts/live | 4 ++--
scripts/live-helpers | 23 +++++++++++++++--------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/scripts/live b/scripts/live
index 9d699b7..e58077c 100755
--- a/scripts/live
+++ b/scripts/live
@@ -1001,12 +1001,12 @@ setup_unionfs ()
;;
esac
- if echo ${PERSISTENCE_METHOD} | grep -qe "\<overlay\>"
+ if is_in_list overlay ${PERSISTENCE_METHOD}
then
overlays="${old_root_overlay_label} ${old_home_overlay_label} ${custom_overlay_label}"
fi
- if echo ${PERSISTENCE_METHOD} | grep -qe "\<snapshot\>"
+ if is_in_list snapshot ${PERSISTENCE_METHOD}
then
snapshots="${root_snapshot_label} ${home_snapshot_label}"
fi
diff --git a/scripts/live-helpers b/scripts/live-helpers
index f604cc4..6a210a2 100644
--- a/scripts/live-helpers
+++ b/scripts/live-helpers
@@ -388,7 +388,7 @@ Arguments ()
then
PERSISTENCE_ENCRYPTION="none"
export PERSISTENCE_ENCRYPTION
- elif echo ${PERSISTENCE_ENCRYPTION} | grep -qe "\<luks\>"
+ elif is_in_list luks ${PERSISTENCE_ENCRYPTION}
then
if ! modprobe dm-crypt
then
@@ -418,6 +418,13 @@ Arguments ()
fi
}
+is_in_list() {
+ local e=${1}
+ shift
+ local l=${@}
+ echo ${l} | grep -qe "^\(.*[[:space:]]\)\?${e}\([[:space:]].*\)\?$"
+}
+
sys2dev ()
{
sysdev=${1#/sys}
@@ -449,9 +456,9 @@ storage_devices()
do
fulldevname=$(sys2dev "${sysblock}")
- if echo "${black_listed_devices}" | grep -qe "^${fulldevname}$" || \
+ if is_in_list ${fulldevname} ${black_listed_devices} || \
[ -n "${white_listed_devices}" ] && \
- echo "${white_listed_devices}" | grep -qve "^${fulldevname}$"
+ ! is_in_list ${fulldevname} ${white_listed_devices}
then
# skip this device entirely
continue
@@ -461,7 +468,7 @@ storage_devices()
do
devname=$(sys2dev "${dev}")
- if echo "${black_listed_devices}" | grep -qe "^${devname}$"
+ if is_in_list ${fulldevname} ${black_listed_devices}
then
# skip this subdevice
continue
@@ -984,7 +991,7 @@ find_persistence_media ()
# in order to probe any filesystem it contains, like we do
# below. activate_custom_mounts() also depends on that any luks
# device already has been opened.
- if echo ${PERSISTENCE_ENCRYPTION} | grep -qe "\<luks\>" && \
+ if is_in_list luks ${PERSISTENCE_ENCRYPTION} && \
is_luks_partition ${dev}
then
if luks_device=$(open_luks_device "${dev}")
@@ -994,14 +1001,14 @@ find_persistence_media ()
# skip $dev since we failed/chose not to open it
continue
fi
- elif echo ${PERSISTENCE_ENCRYPTION} | grep -qve "\<none\>"
+ elif ! is_in_list none ${PERSISTENCE_ENCRYPTION}
then
# skip $dev since we don't allow unencrypted storage
continue
fi
# Probe for matching GPT partition names or filesystem labels
- if echo ${PERSISTENCE_STORAGE} | grep -qe "\<filesystem\>"
+ if is_in_list filesystem ${PERSISTENCE_STORAGE}
then
result=$(probe_for_gpt_name "${overlays}" "${snapshots}" ${dev})
if [ -n "${result}" ]
@@ -1019,7 +1026,7 @@ find_persistence_media ()
fi
# Probe for files with matching name on mounted partition
- if echo ${PERSISTENCE_STORAGE} | grep -qe "\<file\>"
+ if is_in_list file ${PERSISTENCE_STORAGE}
then
result=$(probe_for_file_name "${overlays}" "${snapshots}" ${dev})
if [ -n "${result}" ]
--
1.7.9.5
Attachment:
signature.asc
Description: OpenPGP digital signature