try to create rundir before using them #436

martinetd · 2023-08-03T07:46:20Z

PR #435 makes lockdir default go back to /var/lock, but runtime dir still changed from /run to /run/pppd in commit 66a8c74 ("Let ./configure control the paths for pppd") and is likely to not exist on some distros, in which case the pppdb will not be created.

This is not a big problem but might as well just try to create the directory if it is missing.

Return code of mkdir does not need to be checked as the following open will fail anyway if mkdir failed.

See: #419

ncopa

The if (access(...)) is not needed and racy in theory, as the path can be created between the call to access and mkdir

Better check for and ignore EEXIST, and warn on all other errors to mkdir

martinetd · 2023-08-04T08:42:28Z

for the if access(), I guess I have sequels of lustre filesystems where a mkdir would take a write lock on the parent directory even if the target already exists, which caused endless troubles at $job-1 (thankfully that was fixed to check with a read lock first..)... So I tend to check before create... But I agree for a tmpfs it probably doesn't really matter either way.
Happy to change it if there are strong feelings about it; I don't think the race matters either as it's pretty much the same race between the mkdir and the following create anyway.
for mkdir errors, as said in the commit message/PR description, printing an error or warning will be redundant anyway: if the mkdir failed the open immediately following will fail as well, so logs will be duplicated and don't bring any information.
That can in turn be fixed by not performing the following open if we identified the mkdir failed, but I don't see a reason to do that; it's simpler to just ignore the error and print the warning/error we used to print until now.

paulusmack · 2023-08-07T04:41:25Z

This basically looks fine. I would like the commit message to summarize the issue rather than just saying "PR #435", so that future readers can understand the rationale without needing to have access to github. Likewise "See: #419" also needs to be spelled out in the commit message itself.

martinetd · 2023-08-07T05:12:54Z

Thanks for the review!

This basically looks fine. I would like the commit message to summarize the issue rather than just saying "PR #435"

I had assumed that PR would be merged first as that seemed to be the core of the issue (as discussed in #419) but if this goes first this whole section does not even make sense, I've removed this and added a word about lock dir.

Likewise "See: #419" also needs to be spelled out in the commit message itself.

I've added the title in parenthesis, is that enough? the commit message should be enough to understand the problem regardless of the actual issue anyway.

enaess · 2023-08-15T17:41:55Z

Change here looks fine.

Though I want to ask the question, is the permission mask of 755 what we want here for the folder? Who needs to access this (another instance of pppd?) If so, should one consider a least privilege approach, e.g. only user or group of pppd has access to this folder / file?

enaess · 2023-08-15T17:49:57Z

Come to think about it, this code fails if the path needs to be created recursively. E.g. if someone passed configure --with-lock-dir=/opt/var/locks, and /opt/var doesn't exist where you are trying to create /opt/var/lock/

The chances for this happening is likely low. But keep in mind that the default is to key off the /usr prefix. So you'll end up with /usr/etc/ and /usr/var/locks if you are not careful with the configure switches.

martinetd · 2023-08-15T20:43:18Z

Though I want to ask the question, is the permission mask of 755 what we want here for the folder? Who needs to access this (another instance of pppd?) If so, should one consider a least privilege approach, e.g. only user or group of pppd has access to this folder / file?

fedora seems to use 755 for /run/ppp: https://src.fedoraproject.org/rpms/ppp/blob/rawhide/f/ppp-tmpfiles.conf
I've seen /var/lock as 1777 (alpine) and 755 (fedora), but we shouldn't create open directories so 755 is probably for the best. I don't think any of this is secret and it could be used for coordination with a management process e.g. networkmanager or openwrt scripts

Come to think about it, this code fails if the path needs to be created recursively. E.g. if someone passed configure --with-lock-dir=/opt/var/locks, and /opt/var doesn't exist where you are trying to create /opt/var/lock/

The chances for this happening is likely low. But keep in mind that the default is to key off the /usr prefix. So you'll end up with /usr/etc/ and /usr/var/locks if you are not careful with the configure switches.

Right, the default path for lock would actually be /usr/local/var/run/ppp/lock right now and /usr/local/var/lock after #435 -- which would likely both fail with a single mkdir on most systems, but I don't think (hope?) no package will be using these defaults.
Hopefully all distros automatically pass --localstatedir to configure-based projects so we'll have /var/lock as expected...
I also really meant this as a stop-gap due to the change of path that falls in a tmpfs, but the default isn't a tmpfs so it should be easy enough to create manually if the need does arise.

Anyway, if you really prefer a thorough mkdir-with-parents sure, I can change this path to add a mkdir_p helper somewhere (pppd/utils.c?) and use that in both place?

martinetd · 2023-08-30T00:59:05Z

hi @enaess -- sorry for the direct ping, would you have a minute to answer my reply?
tl;dr is I didn't intend for this to be 100% fool-proof (any mkdir is better than none), but happy to make the mkdir recursive if that helps.

enaess · 2023-08-30T01:07:08Z

The default path if not configured otherwise would fail to be created. I think if you'd taken just a bit more time to fix this, it would be a long lasting fix.

add a recursive mkdir
add a unit test to test this function without running pppd

The fact that the default case would fail is bad. If the default config would succeed, but an other corner case not ... then maybe. Again, quick and dirty fixes may be appropriate for package scrips, distro patches, but if you want to make a long lasting contribution; it would be good to engineer a good fix for all and commit it upstream

martinetd · 2023-08-30T01:28:56Z

No problem, I just wanted to make sure that's the way you want to go before taking the time to do it.
I'll update this PR within the next few days

martinetd · 2023-08-30T03:24:53Z

Updated this PR as suggested.
The first commit adds a mkdir_recursive function and some tests for it (why is this always way more complicated than it should be... I don't know how to make it any simpler); the second commit just uses it.

While I was at it I tried to make it clever and only try to mkdir the parent directory if open failed with ENOENT, but that's more likely to backfire than anything else so happy to just make it unconditional instead.

(BTW if you want my opinion I don't think this problem warrants the complexity this brings, but it's your call; I still have the old commit around if you change your mind)

EDIT: (sorry for update spam on mails; some indentations were a bit weird as I reindented everything automatically after noticed the shiftwidth of 4 space at the end)

enaess

I think if you take a look at the review feedback the change could need a few improvements.

enaess · 2023-08-31T17:53:54Z

pppd/utils_utest.c

+
+#include "pppd-private.h"
+
+/* globals used in test.c... */


Why do you need to declare these globals, they are not used in this file.

These are declared as extern int in headers and used in utils.c -- the test program won't link without these.

enaess · 2023-08-31T17:57:35Z

pppd/tdb.c

@@ -1728,7 +1728,12 @@ TDB_CONTEXT *tdb_open_ex(const char *name, int hash_size, int tdb_flags,
 		goto internal;
 	}

+again:


I am not sure I like the use of label again: and goto, could this be made to fail if it fails and then bail, e.g. use goto fail when it fails to create it recursively.

Can put the whole thing in a while loop but that's a much bigger refactor for something that ought to be a "quick fix"; at this rate it's probably better to just call mkdir_recursive inconditonally first before the open, as it won't fail if the directory already exists.

What do you think?

enaess · 2023-08-31T18:04:17Z

pppd/utils.c

+    char *copy;
+    int rc;
+
+    // optimistically try on full path first to avoid allocation


You could just copy the path into a temporary buffer, and then iterate over that buffer instead of allocating a copy of it. In fact, maybe a quick google search turned up a much more compact solution here: https://stackoverflow.com/questions/2336242/recursive-mkdir-system-call-on-unix

Eh? This will fail with a path bigger than 256 (there's a MAXPATHLEN variable that's usually 4k on unix, but paths can actually be much longer on many filesystems; there's no limit to how long a path can be); you can't get around this without an allocation at which point strdup is just as good.

As for the rest; yes and no. There are two differences between the version you linked and this commit.
This commit starts from the end, and only creates required directories e.g. /var/lock/ppp/ would try to mkdir /var/lock/ppp, if that fails try /var/lock etc and go back. It also checks the result is a directory and checks for errors.
The version you linked would mkdir /var, then /var/lock, then /var/lock/ppp without checking anything.

Now these are just sequels from the lustre filesystem but it's just pushing the work to the kernel, mkdir(/var) when /var already exists should be cheap but it's still more costly than not doing it.
I'm a filesystem developer and don't want my name anywhere near that stackoverflow snippet; if you prefer that version I'll remove this commit so feel free to add it in your name.

Anyway, in our case the parent directory will always exist on distro installs so it makes more sense to go straight for the end -- we'll never allocate anything and use the recursive call, except on very odd setups where the rundir or lockdir is specified somewhere either deeper than usual or mis/not configured.

(Sorry, I do have strong opinions on POSIX filesystem API and this triggered me when I saw this first thing in the morning. For pppd this will only ever be called at init anyway so nothing really matters, but if you prefer an iterative version let's at least keep some error codes... The "improved" version listed in a comment of your stackoverflow link really isn't much shorter than this, the main difference is whether it starts from the start or the end)

enaess · 2023-08-31T18:06:21Z

pppd/utils.c

@@ -844,6 +926,10 @@ lock(char *dev)
 #endif

    while ((fd = open(lock_file, O_EXCL | O_CREAT | O_RDWR, 0644)) < 0) {
+	if (errno == ENOENT && mkdir_recursive(PPP_PATH_LOCKDIR) == 0) {
+	    /* parent dir was missing, retry now */
+	    continue;


So what happens when mkdir_recursive fails? Should we at least log a message?

It'll fall through to errno != EEXIST and print it couldn't create lock file; that's probably enough.
Looking back mkdir_recursive can leave errno set to EEXIST so we probably want to save the original ENOENT errno in that case; I'll adjust this later today.

pppd/utils.c

enaess · 2023-08-31T18:07:46Z

pppd/utils_utest.c

+
+    rmdir(base_dir);
+    return failure;
+}


Seems like you have covered a number of test-cases. Also, you added a new test-suite, that's good.

martinetd · 2023-10-10T02:28:14Z

(rebased after #435 -- I still would prefer a simple mkdir() as in the original version, but this isn't critical anymore as lock dir got moved back to /var/lock. The patch is still useful for /run/pppd for the pppdb so keeping the PR open.)

This will be used in the next commit. A test file for utils has also been added to check mkdir works as intended. Signed-off-by: Dominique Martinet <[email protected]>

paulusmack · 2023-10-10T03:15:31Z

Looks like tdb.c needs to include pppd-private.h to get the declaration of mkdir_recursive.

Runtime dir changed from /run to /run/pppd in commit 66a8c74 ("Let ./configure control the paths for pppd") and is likely to not exist on some distros, in which case the pppdb will not be created. See: ppp-project#419 (lock directory moved in ppp-2.5.0) Signed-off-by: Dominique Martinet <[email protected]>

martinetd · 2023-10-10T03:23:48Z

Looks like tdb.c needs to include pppd-private.h to get the declaration of mkdir_recursive.

Good catch, thank you.

martinetd mentioned this pull request Aug 3, 2023

lock directory moved in ppp-2.5.0 #419

Closed

Neustradamus requested a review from paulusmack August 3, 2023 08:26

ncopa reviewed Aug 4, 2023

View reviewed changes

martinetd force-pushed the mkdir_runtime_lock branch from b621730 to 163485d Compare August 7, 2023 05:09

paulusmack mentioned this pull request Aug 11, 2023

Make lock dir configurable #435

Merged

martinetd force-pushed the mkdir_runtime_lock branch 3 times, most recently from 113a454 to 98a929b Compare August 30, 2023 03:19

martinetd force-pushed the mkdir_runtime_lock branch 2 times, most recently from 8fd2a19 to 5bfd754 Compare August 30, 2023 03:29

enaess reviewed Aug 31, 2023

View reviewed changes

martinetd force-pushed the mkdir_runtime_lock branch 2 times, most recently from d4dd0a9 to b39c834 Compare August 31, 2023 23:34

martinetd force-pushed the mkdir_runtime_lock branch 2 times, most recently from 7d0dc31 to 1ee1505 Compare October 10, 2023 02:25

martinetd changed the title ~~try to create rundir and lockdir before using them~~ try to create rundir before using them Oct 10, 2023

martinetd force-pushed the mkdir_runtime_lock branch from 1ee1505 to d72931e Compare October 10, 2023 03:03

utils: add mkdir_recursive

b0e7307

This will be used in the next commit. A test file for utils has also been added to check mkdir works as intended. Signed-off-by: Dominique Martinet <[email protected]>

martinetd force-pushed the mkdir_runtime_lock branch from d72931e to ea7950a Compare October 10, 2023 03:13

martinetd force-pushed the mkdir_runtime_lock branch from ea7950a to 3a4fec9 Compare October 10, 2023 03:23

paulusmack merged commit c268205 into ppp-project:master Oct 10, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

try to create rundir before using them #436

try to create rundir before using them #436

martinetd commented Aug 3, 2023

ncopa left a comment

martinetd commented Aug 4, 2023 •

edited

Loading

paulusmack commented Aug 7, 2023

martinetd commented Aug 7, 2023

enaess commented Aug 15, 2023

enaess commented Aug 15, 2023 •

edited

Loading

martinetd commented Aug 15, 2023

martinetd commented Aug 30, 2023

enaess commented Aug 30, 2023

martinetd commented Aug 30, 2023

martinetd commented Aug 30, 2023 •

edited

Loading

enaess left a comment

enaess Aug 31, 2023

martinetd Aug 31, 2023

enaess Aug 31, 2023

martinetd Aug 31, 2023

enaess Aug 31, 2023

martinetd Aug 31, 2023

martinetd Aug 31, 2023

enaess Aug 31, 2023

martinetd Aug 31, 2023 •

edited

Loading

enaess Aug 31, 2023

martinetd commented Oct 10, 2023

paulusmack commented Oct 10, 2023

martinetd commented Oct 10, 2023

try to create rundir before using them #436

try to create rundir before using them #436

Conversation

martinetd commented Aug 3, 2023

ncopa left a comment

Choose a reason for hiding this comment

martinetd commented Aug 4, 2023 • edited Loading

paulusmack commented Aug 7, 2023

martinetd commented Aug 7, 2023

enaess commented Aug 15, 2023

enaess commented Aug 15, 2023 • edited Loading

martinetd commented Aug 15, 2023

martinetd commented Aug 30, 2023

enaess commented Aug 30, 2023

martinetd commented Aug 30, 2023

martinetd commented Aug 30, 2023 • edited Loading

enaess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinetd Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinetd commented Oct 10, 2023

paulusmack commented Oct 10, 2023

martinetd commented Oct 10, 2023

martinetd commented Aug 4, 2023 •

edited

Loading

enaess commented Aug 15, 2023 •

edited

Loading

martinetd commented Aug 30, 2023 •

edited

Loading

martinetd Aug 31, 2023 •

edited

Loading