On 5/18/23 17:59, Richard W.M. Jones wrote: > On Thu, May 18, 2023 at 05:42:03PM +0200, Laszlo Ersek wrote: >> On 5/18/23 15:42, Richard W.M. Jones wrote: >>> On Thu, May 18, 2023 at 03:09:42PM +0200, Laszlo Ersek wrote: >>>> Libguestfs uses the >>>> >>>> /dev/VG/LV >>>> >>>> format internally, for identifying LVM logical volumes, but the user might >>>> want to specify the >>>> >>>> /dev/mapper/VG-LV ID >>>> >>>> format with the "--key ID:..." options. >>>> >>>> Introduce unescape_device_mapper_lvm() for turning >>>> >>>> /dev/mapper/VG-LV >>>> >>>> key IDs into >>>> >>>> /dev/VG/LV >>>> >>>> ones, unescaping doubled hyphens to single hyphens in both VG and LV in >>>> the process. >>>> >>>> Call unescape_device_mapper_lvm() from key_store_import_key(). That is, >>>> translate the ID as soon as the "--key" option is processed -- let the >>>> keystore only know about the usual /dev/VG/LV format, for later matching. >>>> >>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2168506 >>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >>>> --- >>>> >>>> Notes: >>>> v2: >>>> >>>> - rewrite without regular expressions [Rich, Laszlo] >>>> >>>> - restructure the commit message >>>> >>>> v1: >>>> >>>> regcomp() must definitely allocate memory dynamically, and we >>>> (intentionally) never free that -- we never call regfree(). I assume >>>> valgrind will catch this as a leak, so we might have to extend >>>> "valgrind-suppressions" in each dependent superproject. However, I'm >>>> unable to run "make check-valgrind" successfully in e.g. virt-v2v even >>>> before these patches; see also >>>> <https://listman.redhat.com/archives/libguestfs/2023-May/031496.html>. >>>> >>>> options/keys.c | 100 ++++++++++++++++++++ >>>> 1 file changed, 100 insertions(+) >>>> >>>> diff --git a/options/keys.c b/options/keys.c >>>> index bc7459749276..52b27369016a 100644 >>>> --- a/options/keys.c >>>> +++ b/options/keys.c >>>> @@ -260,6 +260,105 @@ key_store_add_from_selector (struct key_store *ks, >>>> const char *selector) >>>> return key_store_import_key (ks, &key); >>>> } >>>> >>>> +/* Turn /dev/mapper/VG-LV into /dev/VG/LV, in-place. */ >>>> +static void >>>> +unescape_device_mapper_lvm (char *id) >>>> +{ >>>> + static const char dev[] = "/dev/", dev_mapper[] = "/dev/mapper/"; >>>> + const char *input_start; >>>> + char *output; >>>> + enum { M_SCAN, M_FILL, M_DONE } mode; >>>> + >>>> + if (!STRPREFIX (id, dev_mapper)) >>>> + return; >>>> + >>>> + /* Start parsing "VG-LV" from "id" after "/dev/mapper/". */ >>>> + input_start = id + (sizeof dev_mapper - 1); >>>> + >>>> + /* Start writing the unescaped "VG/LV" output after "/dev/". */ >>>> + output = id + (sizeof dev - 1); >>>> + >>>> + for (mode = M_SCAN; mode < M_DONE; ++mode) { >>>> + char c; >>>> + const char *input = input_start; >>>> + const char *hyphen_buffered = NULL; >>>> + bool single_hyphen_seen = false; >>>> + >>>> + do { >>>> + c = *input; >>>> + >>>> + switch (c) { >>>> + case '-': >>>> + if (hyphen_buffered == NULL) >>>> + /* This hyphen may start an escaped hyphen, or it could be the >>>> + * separator in VG-LV. >>>> + */ >>>> + hyphen_buffered = input; >>>> + else { >>>> + /* This hyphen completes an escaped hyphen; unescape it. */ >>>> + if (mode == M_FILL) >>>> + *output++ = '-'; >>>> + hyphen_buffered = NULL; >>>> + } >>>> + break; >>>> + >>>> + case '/': >>>> + /* Slash characters are forbidden in VG-LV anywhere. If there's >>>> any, >>>> + * we'll find it in the first (i.e., scanning) phase, before we >>>> output >>>> + * anything back to "id". >>>> + */ >>>> + assert (mode == M_SCAN); >>>> + return; >>>> + >>>> + default: >>>> + /* Encountered a non-slash, non-hyphen character -- which also >>>> may be >>>> + * the terminating NUL. >>>> + */ >>>> + if (hyphen_buffered != NULL) { >>>> + /* The non-hyphen character comes after a buffered hyphen, so >>>> the >>>> + * buffered hyphen is supposed to be the single hyphen that >>>> separates >>>> + * VG from LV in VG-LV. There are three requirements for this >>>> + * separator: (a) it must be unique (we must not have seen >>>> another >>>> + * such separator earlier), (b) it must not be at the start of >>>> VG-LV >>>> + * (because VG would be empty that way), (c) it must not be at >>>> the end >>>> + * of VG-LV (because LV would be empty that way). Should any of >>>> these >>>> + * be violated, we'll catch that during the first (i.e., >>>> scanning) >>>> + * phase, before modifying "id". >>>> + */ >>>> + if (single_hyphen_seen || hyphen_buffered == input_start || >>>> + c == '\0') { >>>> + assert (mode == M_SCAN); >>>> + return; >>>> + } >>>> + >>>> + /* Translate the separator hyphen to a slash character. */ >>>> + if (mode == M_FILL) >>>> + *output++ = '/'; >>>> + hyphen_buffered = NULL; >>>> + single_hyphen_seen = true; >>>> + } >>>> + >>>> + /* Output the non-hyphen character (including the terminating NUL) >>>> + * regardless of whether there was a buffered hyphen separator >>>> (which, >>>> + * by now, we'll have attempted to translate and flush). >>>> + */ >>>> + if (mode == M_FILL) >>>> + *output++ = c; >>>> + } >>>> + >>>> + ++input; >>>> + } while (c != '\0'); >>>> + >>>> + /* We must have seen the VG-LV separator. If that's not the case, >>>> we'll >>>> + * catch it before modifying "id". >>>> + */ >>>> + if (!single_hyphen_seen) { >>>> + assert (mode == M_SCAN); >>>> + return; >>>> + } >>>> + } >>>> +} >>> >>> So this code can never return an error? >> >> It surely can; the one error mode is exactly the same as it was in v1 >> (the regex version). Namely, the error mode is that the input pathname >> in "id" does not match the expected form (i.e., /dev/mapper/VG-LV), and >> then we don't modify "id" at all, we let "id" be added to the keystore >> exactly as the user specified it. >> >> The error mode is activated whenever we reach any of the explicit return >> statements in the function: >> >> - when the first STRPREFIX call falls >> >> - or when any of the other three "return" statements are reached (which >> can only be reached during the scanning phase, M_SCAN; that is, *before* >> we write anything at all into "id"). >> >> The point of separating M_SCAN from M_FILL is to verify the full format >> match before we start modifying "id" in-place. The traversal (parsing) >> through "VG-LV" is exactly the same in both cases, but in the first >> pass, we just check. If that completes fine, we repeat the same >> traversal, but then we also modify "id" in-place. >> >> The translation only ever contracts VG-LV (it produces at most as many >> bytes as it consumes), and the write point is always to the left of the >> read point, so the in-place modification, including the final >> NUL-termination, is safe. >> >>> eg if the input was "A-B-C", >>> I think it would translate the string to "A/B-C" which is an output >>> but the input seems like it was wrong. >> >> If "id" contains the string "/dev/mapper/A-B-C", then it *mismatches* >> the expected form "/dev/mapper/VG-LV" just as much as an ID containing >> "/dev/sda2" would mismatch the expected form. >> >> In either of those cases, "id" does not get modified, and gets added to >> the keystore precisely as the user specified it on the command line. >> >> We don't say "this ID is wrong, error!". We don't know what the ID is, >> we only know what it is *not*. We only say "this is not a >> /dev/mapper/VG-LV specification, we just don't know what it is supposed >> to be, so we won't touch it, we'll add it as it is, to the keystore". >> >> That's exactly the behavior (= blind addition) that we have right now, >> i.e., pre-patch. > > OK got it, in that case: > > Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
Thank you; commit range 38e6988c1864..b636c3f20a1b. Laszlo > >> unescape_device_mapper_lvm() either modifies "id" in place (if "id" on >> input has the expected form), or leaves "id" alone. >> >> The logic following the unescape_device_mapper_lvm() *call site* doesn't >> know which way the function went, but it does not *need* to know. >> >>> Or is it that only the VG is >>> escaped? (I don't imagine there is some specification for /dev/mapper >>> device names.) >> >> "/dev/mapper/A-B-C" simply cannot be interpreted according to the >> "/dev/mapper/VG-LV" scheme. For such an interpretation to be possible, >> we'd have to have one of the following strings in "id": >> >> - "/dev/mapper/A--B-C" --> "/dev/A-B/C" >> - "/dev/mapper/A-B--C" --> "/dev/A/B-C" >> >> If the user specifies (for example) "/dev/mapper/A-B-C", we reject that, >> so the ID remains "/dev/mapper/A-B-C", and that is what gets added to >> the keystore -- exactly as it happens pre-patch. >> >> If the user specifies (for example) "/dev/mapper/A--B--C", we also >> reject that (there is no separator to tell us what is the VG and what is >> the LV), so the ID remains "/dev/mapper/A--B--C", and that is what gets >> added to the keystore -- exactly as it happens pre-patch. >> >> FWIW, this aspect is identical between version 1 and version 2 of this >> patch: in v1, if the regex didn't match, then "id" would simply not be >> rewritten, and would get added to the keystore as specified by the user. >> The v1->v2 update is an implementation detal, internal to >> unescape_device_mapper_lvm() -- it only affects how the expected form is >> recognized (regex versus manual parsing), and how a matching input is >> translated (the output being constructed from matched regex >> subexpressions, versus byte-wise buffering and flushing integrated with >> scanning). > > Thanks, > > Rich. > _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
