| Summary: | sys-fs/cryptsetup: startup scripts are not POSIX compliant | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | Martin Väth <martin> |
| Component: | Current packages | Assignee: | Gentoo's Team for Core System packages <base-system> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | contyk, immerdabeiundnie, markus.heier, michael.schachtebeck, michal.terepeta, nikoli, thomas.kreuzer |
| Priority: | High | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | 273039 | ||
| Bug Blocks: | |||
| Attachments: |
Patch to make files/* in cryptsetup-1.0.6-r2 POSIX compliant
The above patch with two bugs fixed The above patch, also with the read timeout and -n1 Previous patch with read -u fixed and added bells and whistles Patch for 1.1.3-r2 and 1.2.0-r1 Update patch for new cryptsetup files Fix timeout issue for new dmcrypt.rc |
||
|
Description
Martin Väth
2009-01-19 17:51:52 UTC
Created attachment 179008 [details, diff]
Patch to make files/* in cryptsetup-1.0.6-r2 POSIX compliant
The patch is completely untested - I do not use cryptsetup in this manner
on my system and do not want to make such a setup just to test the scripts.
I just replaced the bashisms which I saw by POSIX compliant code.
Only for the "read -n 1 -t ..." there is no easy POSIX substitute;
I have just omitted the timeout parameter but do not know whether this leads
to reasonable behavior - perhaps more effort has to be spent here.
(In reply to comment #1) > Created an attachment (id=179008) [edit] > Patch to make files/* in cryptsetup-1.0.6-r2 POSIX compliant I have tested the patch, and I encountered the following error: cryptsetup will be called with: luksOpen /dev/hda4 crypt-home Command failed: No key available with this passphrase [I didn't enter a passphrase] Failure running cryptsetup Checking swap is not LUKS Source "" for crypt-swap missing, skipping ... And then he starts again with: cryptsetup will be called with: luksOpen /dev/hda4 crypt-home Enter passphrase: and then it seems to work, at least with the home partition, I don't get a swap. thank you very much so far for this patch. Glancing over the patch once more, I found one mistake:
In Line 74 of the patch (Line 83 of 1.0.6-dm-crypt-start.sh)
there should be
+ key=${key%%:*}
instead of
+ key=${key##*:}
(apparently, I did not read the original bash code carefully enough).
This sounds like it could be related with the problem you observed.
(In reply to comment #3) > Glancing over the patch once more, I found one mistake: > In Line 74 of the patch (Line 83 of 1.0.6-dm-crypt-start.sh) > there should be > + key=${key%%:*} > instead of > + key=${key##*:} > (apparently, I did not read the original bash code carefully enough). > > This sounds like it could be related with the problem you observed. > Sadly, no. I changed the line in /lib/rcscripts/addons/dm-crypt-start.sh though, at line 87 then. Thanks again, so far. (In reply to comment #4) > Sadly, no. > I changed the line in > > /lib/rcscripts/addons/dm-crypt-start.sh > > though, at line 87 then. > > Thanks again, so far. > Oh, and btw, it still works with bash. Created attachment 179385 [details, diff]
The above patch with two bugs fixed
The attached patch fixes the previous bug and removes another bashism I
overlooked.
However, I think this is not related with your problem. Since I cannot see
another bashism in the sources, I cannot imagine why it still works with
bash but not with dash. Maybe inserting "set -x" in the start function of the initscript and redirecting the output does show an informative difference?
your patches have undesirable behavior change when it comes to reading the input the way to increment something in POSIX sh is: : $((i+=1)) the color vars should already be set rather than having them redeclared locally (In reply to comment #7) > your patches have undesirable behavior change when it comes to reading the > input Do you see which of the patched lines changes the behavior? I really cannot find it. > the way to increment something in POSIX sh is: > : $((i+=1)) This is a little bit less compatible than the more cumbersome way (e.g. earlier dash version could only use variables in arithmetic expansion if they were preceded with $. In fact, the POSIX specification is a bit unclear here, IMHO). Moreover, even the informative example in http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html# tag_02_06_04 uses the method which I used. > the color vars should already be set rather than having them redeclared > locally Since I cannot test, I did not try to modify anything except the bashisms of the original script (which also declared these variables locally) - I just changed the escape codes in these definitions to be more standard. you changed the read_abort() func to not respect arguments and dropped all arguments to the read func (In reply to comment #9) > you changed the read_abort() func to not respect arguments and dropped all > arguments to the read func All this should change is that there is no automatic timeout (which I have described - this should not be related with Thomas' problems): The first argument is respected; the other arguments involve only the timeout which (as I have written) has no easy POSIX substitute. the point of converting to POSIX is to replace bashisms with functionality equivalent POSIX code. these changes are not functionally equivalent. (In reply to comment #11) > the point of converting to POSIX is to replace bashisms with functionality > equivalent POSIX code. these changes are not functionally equivalent. Which of those sentences were unclear: : Only for the "read -n 1 -t ..." there is no easy POSIX substitute; : I have just omitted the timeout parameter but do not know whether this leads : to reasonable behavior - perhaps more effort has to be spent here. So I rephrase: I do not think that this timeout is so important that it justifies so much work, at least not before the patch is seriously tested and somebody means that the timeout functionality is crucial. If you think differently, feel free to do the additional work and implement a function which reads with timeout. But it is of course easier to just complain that I didn't do _all_ of cryptsetup's maintainer job... It would be more helpful for the users to find the _real_ reason for the behavior change in the patch which Thomas has observed (and which rather obviously has nothing to do with an omitted timeout-argument for read). if there is no timeout, then the boot will sit there indefinitely Created attachment 181693 [details, diff] The above patch, also with the read timeout and -n1 > if there is no timeout, then the boot will sit there indefinitely Yes it will sit there until the user answers a question which is rather crucial for the boot. So I doubt that this change is very important. However, I was interested anyway in how to implement the read options -n1 and -t cleanly in POSIX. So after getting a hint from usenet, I attach now a version of the patch which also respects the timeout and has -n1 built-in. mounting of crypt partitions might be critical in your setup, but that doesnt mean it's critical for everyone. having a remote system with a few encrypted partitions that are not critical to getting the system online is certainly plausible. i know because i have such a system myself. i'll check out the new script when i get a chance, thanks. Can anyone confirm, that the patch is working correctly? (In reply to comment #16) > Can anyone confirm, that the patch is working correctly? > I just tested the current patch and it works as expected. No misbehaviour, all seems to be fine, also when using dash as /bin/sh. Works fine for me, thank you very much for providing the patch. Hi there, I somehow had forgotten about this bug, however, I cannot report complete success with dash, it works good for an encrypted hard disk partition, yet, the script does not create an encrypted swap. It gives me the error: read 310: illegal option -u In line 276 of dm-crypt-start.sh I can find the command "read -u 3 targetline .." I tried to find the problem but was unsure what to do exactly, simply remove the "-u" ? Anyhow, I would be great if someone knew what to do. Many thanks so far :) regards, Thomas Created attachment 204660 [details, diff]
Previous patch with read -u fixed and added bells and whistles
Nope, you must redirect the input of read to fd#3.
The attached patch should fix this, undoes a previous unnecessary change,
and also has an improved read_abort() (IMHO nicer than the original).
However, except for trying read_abort() once, it is untested (by me).
Thank you very much Martin, now it works perfectly with dash (as far as my test case is concerned) Now this script only needs to be incorporated into portage :) regards, Thomas unfortunately, this patch doesn't work with cryptsetup-1.2.0-r1
so i replaced all "[[ .. ]]" with "[ .. ]" in dm-crypt-start.sh and made the change at the "read -u" point.
no when starting dm-crypt, it tries to make the swap-device twice, which fails:
/etc/init.d/dmcrypt start
* Setting up dm-crypt mappings ...
* Checking swap is not LUKS
* dm-crypt map crypt-swap ...
* cryptsetup will be called with : -c aes -h sha1 -d /dev/urandom create crypt-swap /dev/disk/by-id/ata-SAMSUNG_HD103UJ_S13PJ1DQ205922-part2 [ ok ]
* Running pre_mount commands for crypt-swap ...
mkswap: /dev/mapper/crypt-swap: warning: don't erase bootbits sectors
on whole disk. Use -f to force. [ ok ]
* Checking swap is not LUKS
* dm-crypt map crypt-swap ...
* cryptsetup will be called with : -c aes -h sha1 -d /dev/urandom create crypt-swap /dev/disk/by-id/ata-SAMSUNG_HD103UJ_S13PJ1DQ205922-part2
Das Gerät crypt-swap existiert bereits.
* failure running cryptsetup [ !! ]
* Failed to setup dm-crypt devices [ !! ]
* ERROR: dmcrypt failed to start
what else do i need to change?
and once again with the english cryptsetup output, sorry:
LANG="en" /etc/init.d/dmcrypt start
* Setting up dm-crypt mappings ...
* Checking swap is not LUKS
* dm-crypt map crypt-swap ...
* cryptsetup will be called with : -c aes -h sha1 -d /dev/urandom create crypt-swap /dev/disk/by-id/ata-SAMSUNG_HD103UJ_S13PJ1DQ205922-part2 [ ok ]
* Running pre_mount commands for crypt-swap ...
mkswap: /dev/mapper/crypt-swap: warning: don't erase bootbits sectors
on whole disk. Use -f to force. [ ok ]
* Checking swap is not LUKS
* dm-crypt map crypt-swap ...
* cryptsetup will be called with : -c aes -h sha1 -d /dev/urandom create crypt-swap /dev/disk/by-id/ata-SAMSUNG_HD103UJ_S13PJ1DQ205922-part2
Device crypt-swap already exists.
* failure running cryptsetup [ !! ]
* Failed to setup dm-crypt devices [ !! ]
* ERROR: dmcrypt failed to start
Created attachment 259555 [details, diff]
Patch for 1.1.3-r2 and 1.2.0-r1
The adopted pathc. As the other patches, it is completely untested.
thank u martin for quick reply, but either i'm too stupid or this is only for 1.1.3. i go to /lib/rcscripts/addons and make "patch < ./posix.patch" then i have to enter the filenames, but the patches fail almost all. I applied the failed patches manually, but it still tries to startup my cryptswap twice. Hello martin, after some own investigations, I added a "continue" at the end of the "target=*|swap=*)" case-section, in dm-crypt-start.sh. now it doesn't try to start the swap device twice, but I get a ewarning: * Ignoring setting outside target/swap section: source='/dev/disk/by-id/ata-SAMSUNG_HD103UJ_S13PJ1DQ205922-part2' I'm not sure, it's working like it should, but, I don't think, that this line should be there. Do you need my script as it is now, to take a look at it? (In reply to comment #25) > i go to /lib/rcscripts/addons and make "patch < ./posix.patch" This is wrong: The patch is meant to be applied in the "files/" directory (you must copy the whole ebuild directory to your local overlay, first). (In reply to comment #26) > I added a "continue" at the end of the "target=*|swap=*)" case-section I have not checked the script's logic, I just "mechanically" converted it into equivalent POSIX code. It seems to me that if a "continue" is needed there, this is not related to the POSIX conversion (i.e. you would have the same problem already with the original script with bash). If this is the case, you should perhaps open a new bug. (In reply to comment #27) > (In reply to comment #25) > > i go to /lib/rcscripts/addons and make "patch < ./posix.patch" > > This is wrong: The patch is meant to be applied in the "files/" directory > (you must copy the whole ebuild directory to your local overlay, first). ok, did that now and everything is fine. patch is working and after reinstall of cryptsetup, the scripts are working with dash. It seemed to me, that i didn't have the up to date scripts for some reason, they must have changed, but there was no reinstall of cryptsetup, so i had old scripts in /lib/rcsripts/addons. thx for helping Created attachment 259747 [details, diff]
Update patch for new cryptsetup files
Patch for 1.1.3-r3 (and 1.2.0-r1)
Please would somebody be so kind and finally push the patch to portage? I do not want to redo the work all again if somebody else writes an "update" for the non-fixed version! AFAIK except for some special mysql package, this is the last init-script which was not yet fixed, although the patch (with _equivalent_ behaviour!) is now available on this bug for almost two years. *** Bug 367937 has been marked as a duplicate of this bug. *** Is there any way to get this or some similar patch into the tree? I'm just curious, because it is a really annoying bug that hits me just when I already forgot about it... 1.4.1 should be mostly POSIX compliant. the read timeout issue remains at least. (In reply to comment #33) > the read timeout issue remains at least. The "read timeout issue" should be solved in my patches since comment #14 (by using stty). (In reply to comment #34) and your patches are not applicable anymore, so my comment stands: the timeout issue remains in the tree Created attachment 298255 [details, diff]
Fix timeout issue for new dmcrypt.rc
The attached patch fixes the timeout (and read -n) issue with current dmcrypt.rc
In addition two other minor bugs in read_abort() are fixed:
1. Obviously, the "shift" was meant to be after printing of the message.
2. Since "$ans" is at most one character, it makes no sense to test for "yes".
Comment on attachment 298255 [details, diff]
Fix timeout issue for new dmcrypt.rc
i don't like having to screw with stty, but there's probably no way around it in order to be POSIX compliant ...
i'll have to toy with it, but this is probably largely what will be merged ...
(In reply to comment #37) > i don't like having to screw with stty Me neither, but although I haven't checked the bash source code, I am quite convinced that also bash will internally do the same for read -n or read -t. What I especially do not like is that the stty setting is not bound to the application, so that you have to catch traps unless you want to risk that the user is left with modified a stty after a Ctrl-C (IIRC only zsh resets the stty settings at the prompt). should be all set now in the tree; thanks for the report! Commit message: Replace `read -t` with stty hacks http://sources.gentoo.org/sys-fs/cryptsetup/cryptsetup-1.4.2.ebuild?rev=1.1 http://sources.gentoo.org/sys-fs/cryptsetup/files/dmcrypt.rc?r1=1.3&r2=1.4 |
