Improve Maildir file uniqueness in qmail-local.c by xenotrope · Pull Request #90 · notqmail/notqmail

xenotrope · 2019-08-26T22:47:52Z

Some operating systems quickly recycle PIDs, which can lead to
collisions between Maildir-style filenames, which must be unique and
non-repeatable within one second. This patch is just a means of updating
qmail-local to use the format of the revised Maildir protocol.

It uses four unique identifiers:

inode number of the file written to Maildir/tmp
device number of the file written to Maildir/tmp
time in microseconds
the PID of the writing process

A Maildir-style filename would look like the following:

In Maildir/tmp:
time.MmicrosecondsPpid.host
In Maildir/new:
time.IinodeVdeviceMmicrosecondsPpid.host

Additionally, this patch further comforms to the revised Maildir
protocol by looking through the hostname for instances of '/' and ':',
replacing them with "\057" and "\072", respectively, when writing it to
disk. Special thanks go to Matthias Andree for design & sanity-checking.

mbhangui · 2019-08-27T04:04:29Z

fmt_xlong() should be moved to fmt_xlong.c.
we should add the fmt_xlong() proto to fmt.h

DerDakon · 2019-08-27T06:33:35Z

 pid = getpid();
 host[0] = 0;
 gethostname(host,sizeof(host));
+


one could do an initial allocation for hostname here, so it has enough room to hold "host" first, which should be enough for all sane cases anyway. Or maybe just do a scan through host if it has any of the special chars and just copy over the whole string otherwise (i.e. always).

The most simple version, which will give us enough room for the unchanged host, which is enough for the usual case, is:

stralloc_readyplus(&hostname, str_len(host));

POSIX gethostname does not null-terminate the buffer if it was too long, so I recommend:

host[sizeof host - 1] = 0;

The best solution here is for users to adopt OpenBSD's gethostname(3): "The returned name is always NUL terminated." I've updated this to use a stralloc and then store the hostname in the sa.s array and write '\0' in the last byte. For hostnames with 256 or more characters, this will truncate it to the first 255.

This still misses to preallocate hostname as shown above.

One could also think of not preallocating and simply not touching hostname at all until we actually found a position where it needs to be recoded, and only then allocate it and copy the whole prefix at once. This would however make the code slightly more complicated.

DerDakon · 2020-04-08T04:59:27Z

 gethostname(host,sizeof(host));
+
+ s = host;
+ for (loop = 0; loop < str_len(host); ++loop)


The style used everywhere else is to put the { on the line of the control statement.

schmonz · 2020-05-19T15:40:38Z

I'd like to see this rebased and merged sometime soon after 1.08 is out, so I've assigned it the 1.09 milestone. @xenotrope, might you have attention for this sometime in the next week-ish?

DerDakon · 2020-05-28T06:26:49Z

You now add fmt_xlong() back to fmt.h after we removed it in #126. Please add it with complete function signature.

DerDakon

It would be awesome if that could be accompanied by a unittest for at least maildir_child() and fmt_xlong().

DerDakon · 2020-06-10T05:34:10Z

-   s += fmt_ulong(s,time); *s++ = '.';
-   s += fmt_ulong(s,pid); *s++ = '.';
-   s += fmt_strn(s,myhost,sizeof(myhost)); *s++ = 0;
+   s += fmt_ulong(s,time.tv_sec); *s++ = '.';


This adds both '.' and 'M', I don't thinkg that is intentional.

The old Maildir format is "time.pid.hostname:flags". By replacing "pid" with more stuff, including a sub-second time measurement, this patch changes it to "time.MμsecPpid.host:flags". So time.tv_sec and time.tv_usec are both needed, but one comes before the first dot and the other comes after it.

Please put all of them in different lines to improve readability. If you want to group the sections than insert newlines.

Well-behaving mail software would ignore all the pseudo-random part of the name, so I suppose adding letters in addition to . and 0-9 is fine.
Also, I think I remember there are already software around using letters in there.

DerDakon · 2020-06-10T05:35:36Z

+/* writes ulong u in hex to char *s, does not NULL-terminate */
+unsigned int fmt_xlong(s,u) char *s; unsigned long u;
+{
+ unsigned int len; unsigned long q; unsigned long c;


Please unwrap them also, which also gives the opportunity to initialize them immediately and get rid of the next line entirely.

fmt_xlong() was removed from qmail-local.c a while back and this regression will be fixed in a new push.

DerDakon · 2020-06-10T05:39:19Z

 pid = getpid();
 host[0] = 0;
 gethostname(host,sizeof(host));
+


This still misses to preallocate hostname as shown above.

One could also think of not preallocating and simply not touching hostname at all until we actually found a position where it needs to be recoded, and only then allocate it and copy the whole prefix at once. This would however make the code slightly more complicated.

schmonz · 2020-06-10T12:33:16Z

- unsigned long time;
- char myhost[64];
+ struct timeval time;
+ char host[64];


In 1.08 we renamed this local host to myhost to avoid shadowing the same-named global. Please catch up to the rename to reduce this diff.

P.S. I recognize we’ve made this PR more labor-intensive for you by changing a bunch of stuff out from under it. Sorry about this! And please keep going — I think we’re pretty close to making maintenance of this code everyone’s job.

Hopefully maintaining this code doesn't become necessary for anyone, at least not in a long-term capacity. While I deeply appreciate your apology, (really, thank you!) it's not necessary. A number of these new changes seem[1] to be because the "git rebase master" undid some commits I've made in an earlier version of this branch to address previous coding concerns. I blame myself. And also Junio Hamano. Any chance somebody has the temerity to make "darcshub.com" or "Codevillehub.com"?

[1] I'm not entirely sure, because everything seemed like it was OK before I pushed, but... my mother said that once and she was gravely mistaken.

DerDakon · 2020-06-19T19:18:40Z

 void temp_qmail(fn) char *fn;
 { strerr_die5x(111,"Unable to open ",fn,": ",error_str(errno),". (#4.3.0)"); }

+/* writes ulong u in hex to char *s, does not NULL-terminate */


This function now lives in it's own file, so drop it here.

Some operating systems quickly recycle PIDs, which can lead to collisions between Maildir-style filenames, which must be unique and non-repeatable within one second. This patch is just a means of updating qmail-local to use the format of the revised Maildir protocol. It uses four unique identifiers: * inode number of the file written to Maildir/tmp * device number of the file written to Maildir/tmp * time in microseconds * the PID of the writing process A Maildir-style filename would look like the following: In Maildir/tmp: time.MmicrosecondsPpid.host In Maildir/new: time.IinodeVdeviceMmicrosecondsPpid.host Additionally, this patch further comforms to the revised Maildir protocol by looking through the hostname for instances of '/' and ':', replacing them with "\057" and "\072", respectively, when writing it to disk. Special thanks go to Matthias Andree for design & sanity-checking.

DerDakon · 2020-10-19T15:37:49Z

+ unsigned long c;
+ len = 1; q = u;
+ while (q > 15) { ++len; q /= 16; }
+ if (s)


Please reformat this block: put the { in the line of the if() and unwrap the do-while loop.

DerDakon · 2020-10-19T15:39:01Z

+ hostsa.s[hostnamemaxlen-1] = 0;
+
+ s = hostsa.s;
+ for (loop = 0; loop < str_len(hostsa.s); ++loop)


please use strlen() instead of str_len() in new code.

DerDakon · 2020-10-19T15:40:50Z

-   s += fmt_ulong(s,time); *s++ = '.';
-   s += fmt_ulong(s,pid); *s++ = '.';
-   s += fmt_strn(s,myhost,sizeof(myhost)); *s++ = 0;
+   s += fmt_ulong(s,time.tv_sec); *s++ = '.';


Please put all of them in different lines to improve readability. If you want to group the sections than insert newlines.

josuah

Nothing to add to what @DerDakon and @leahneukirchen said.

josuah · 2022-05-05T18:00:40Z

+   if (hostsa.s[loop] == '/')
+    {
+     if (!stralloc_cats(&hostname,"\\057")) temp_nomem();
+     continue;
+    }
+   if (hostsa.s[loop] == ':')
+    {
+     if (!stralloc_cats(&hostname,"\\072")) temp_nomem();
+     continue;
+    }
+   if (!stralloc_append(&hostname,s+loop)) temp_nomem();


Just making sure: there is no program in qmail that tries to decode these escape codes?
It is just in case any of those are encountered?
Why not hard fail instead? It is not expected to have these characters in a host name in the first place, so it might not bother anyone if it stops in these cases.

<name> ::= <let>[*[<let-or-digit-or-hyphen>]<let-or-digit>]

https://datatracker.ietf.org/doc/html/rfc952

Although, it may happen: hostname / works just fine here.

The encoding of / and : is specified in https://cr.yp.to/proto/maildir.html, there's no need to decode it.

josuah · 2022-05-05T18:03:54Z

-   s += fmt_ulong(s,time); *s++ = '.';
-   s += fmt_ulong(s,pid); *s++ = '.';
-   s += fmt_strn(s,myhost,sizeof(myhost)); *s++ = 0;
+   s += fmt_ulong(s,time.tv_sec); *s++ = '.';


Well-behaving mail software would ignore all the pseudo-random part of the name, so I suppose adding letters in addition to . and 0-9 is fine.
Also, I think I remember there are already software around using letters in there.

schmonz · 2023-09-22T14:13:27Z

@xenotrope, would you be comfortable with me handling the rebasing, etc. and getting this PR merged? I can make sure to keep the change attributed to you (as the patch itself is). We want this one in 1.09 and I'd be happy to take it from here, ideally with your permission.

aaeegg · 2024-09-01T03:07:34Z

The wall-clock time from gettimeofday can sometimes be non-monotonic. On systems that have a monotonic uptime count readable by clock_gettime(CLOCK_MONOTONIC), how about putting that in the delivery identifier in addition to the gettimeofday time?

I propose expressing this as Onm, where n is (in hexadecimal) the tv_sec from clock_gettime(CLOCK_MONOTONIC) and m is (in hexadecimal, zero-padded on the left to 8 digits) the tv_nsec.

xenotrope requested a review from schmonz August 26, 2019 22:47

DerDakon requested changes Aug 27, 2019

View reviewed changes

DerDakon reviewed Sep 9, 2019

View reviewed changes

Comment thread qmail-local.c Outdated

DerDakon reviewed Oct 26, 2019

View reviewed changes

Comment thread fmt_xlong.c

DerDakon requested changes Apr 8, 2020

View reviewed changes

schmonz added this to the 1.09 milestone May 19, 2020

DerDakon reviewed May 19, 2020

View reviewed changes

Comment thread fmt_xlong.c Outdated

schmonz mentioned this pull request May 21, 2020

Fix leap second handling on systems running TAI #89

Draft

leahneukirchen approved these changes May 23, 2020

View reviewed changes

DerDakon mentioned this pull request May 28, 2020

qmail-local: close race window when creating file in tmp/ #119

Merged

xenotrope force-pushed the notqmail-maildir-uniqueness branch from d1e635b to 07f0cef Compare June 10, 2020 02:12

DerDakon requested changes Jun 10, 2020

View reviewed changes

schmonz reviewed Jun 10, 2020

View reviewed changes

schmonz requested review from alanpost, jlmuir, josuah, leahneukirchen and mbhangui June 10, 2020 12:36

leahneukirchen approved these changes Jun 10, 2020

View reviewed changes

mbhangui approved these changes Jun 10, 2020

View reviewed changes

DerDakon reviewed Jun 19, 2020

View reviewed changes

xenotrope force-pushed the notqmail-maildir-uniqueness branch from 93c936e to 5c66122 Compare October 19, 2020 14:40

DerDakon requested changes Oct 19, 2020

View reviewed changes

schmonz removed the request for review from alanpost November 3, 2020 14:55

josuah approved these changes May 5, 2022

View reviewed changes

schmonz modified the milestones: 1.09, 1.10 Oct 13, 2023

Conversation

xenotrope commented Aug 26, 2019

Uh oh!

mbhangui commented Aug 27, 2019

Uh oh!

DerDakon Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmonz commented May 19, 2020

Uh oh!

Uh oh!

DerDakon commented May 28, 2020

Uh oh!

DerDakon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmonz commented Sep 22, 2023

Uh oh!

aaeegg commented Sep 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DerDakon Aug 27, 2019 •

edited

Loading