Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() #13389
Conversation
|
There's a few problems with this:
Tend toward NACK. Better to set the permissions on the data directory once, manually, it's not going to touch them after that. |
|
Or, if this is a big issue, maybe do it in both places? |
|
Would be nice to know when this regression was introduced. If it never did anything, I guess it can be removed? If it is not going to be removed, this must come with functional tests that verify the behavior. |
|
Appreciate the feedback, I will look into these later - please let me know if you have any other suggestion as well. Thank you! |
|
Had a look at the bitcoin-qt as suggested - it looks like if there was a regression it could be because it potentially generates datadir on its own, without calling GetDefaultDataDir(), see comments in the commit above. Tested by running bitcoind, and bitcoin-qt and checking datadir. The existing unit tests, don't appear to rely on either of the above utility function calls - should I still go ahead and add unit tests for this anyway? thanks! |
|
@n2yen You can add a python script into functional test, it allow you to run a node to test this. |
It definitely works, but not for the initial directory creation. It works for files created after that, and that's what it's intended for (block file data sharing, basically). |
|
@Iaanwj can you clarify if we should also leave the old the old code where it was also (in AppInitBasicSetup )? i'm not sure, thank you for your feedback! |
|
Oh I get the problem now, it's not OK, yes, it should definitely be moved so that the data directory is also created with that umask too. I'm still not sure There's an inherent chicken-and-egg problem with the Also agree this needs a test, because apperently this is another initialization issue with strict ordering requirements. |
|
So to show the current code should not naively be removed (nor applied for $ src/bitcoind -datadir=/tmp/wallet1 -regtest
$ stat /tmp/wallet1/regtest/wallets/wallet.dat
Access: (0600/-rw-------)After removing the $ src/bitcoind -datadir=/tmp/wallet2 -regtest
$ stat /tmp/wallet2/regtest/wallets/wallet.dat
Access: (0660/-rw-rw----)(wallet is group-readable now, the exact result depends on the umask in the user's environment of course, but this is undesirable) |
|
@Iaanwj ok, so we're in agreement, that the umask in AppInitBasicSetup() may not be early enough. I agree that the umask cannot just be in bitcoind.cpp, but also in bitcoin-qt or some common code that can be called earlier... I will also be adding tests in the way suggested by @ken2812221. |
af589c6 to
e429ba8
Compare
|
Updated commit with tests to verify permissions of regtest directory, and umask is set as early as sensible in both bitcoind and bitcoin-qt. @laanwj , @MarcoFalke can I please get your feedback? |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
| The last travis run for this pull request was 50 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
|
@practicalswift thank you! |
|
@practicalswift timed out again, this time, didn't even get a chance to run the feature tests! |
test/functional/feature_sysperms.py
Outdated
| self.restart_node(0) | ||
| datadir = self.nodes[0].datadir | ||
|
|
||
| self.log.info("Running test without -sysperms option - assuming umask of '0o77'") |
There was a problem hiding this comment.
you're right, should be '077'. What did you make of -sysperm option, is it still needed?
There was a problem hiding this comment.
What did you make of -sysperm option, is it still needed?
My concerns about -sysperms are in the note here #15902.
There was a problem hiding this comment.
does it makes sense to close this PR - perhaps replace it with simply removing the option? i suppose the tests might still be useful...
|
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits |
|
rebased! |
|
|
|
I don't think my original comments have been addressed (edit: the GUI one is). This still doesn't allow setting sysperm in the configuration file, only on the command line. It also complicates the initialization order. I'm starting to think it would be better to remove the |
|
I underestimated the fact that one can no longer use the |
|
ok, will have a look. |
…cSetup() as it is too late there. When the option -sysperms=false (default) is enabled, the created datadirs does not take into account the default umask setting of '077'. Fix moves the umask operation as early as possible inside of bitcoind, and bitcoin-qt. This was needed because by the time AppInitBasicSetup() is called, datadir, has typically already been created. Calling umask earlier ensures any filesystem writes that follows has the correct umask set. Tests: feature_sysperms.py - validates datadir permissions where -sysperms=false (default) Notes: '-sysperms' option requires '-disablewallet' - add check for windows platform, feature_nosysperms.py will explicitly skip the test if 'Windows' platform is detected. if bitcoind is built with NO_WALLET=1. That is -sysperms will fail if not accompanied with -disablewallet option
|
Closing in favour of #17127 (which is probably superior). |
When the option -sysperms=false (default) is enabled, the created datadirs doesn't take into account the default umask setting of '077'.
The fix moves the umask operation to just after parameters are parsed during AppInit()