-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import Bruce Guenter's qmail-qfilter #227
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a change to COPYRIGHT
?
This would get added at the time of a new release maybe.
It is really good that there are well-established filtering mechanism, that saves us to add our own, and will permit many changes without patches, maintained as "plugins" or "filters".
Result of make brokemaster BRANCH=schmonz-qmail-qfilter-osf
on https://github.com/josuah/notqmail-patch:
schmonz-qmail-qfilter-osf broke ext-todo
schmonz-qmail-qfilter-osf broke smtp-tls
smtp-tls: failed hunks
Makefile
@@ -781,7 +781,7 @@
forward preline condredirect bouncesaying except maildirmake \
maildir2mbox install instpackage instqueue instchown \
instcheck home home+df proc proc+df binm1 binm1+df binm2 binm2+df \
-binm3 binm3+df
+binm3 binm3+df update_tmprsadh
load: \
make-load warn-auto.sh
hier.c.rej:
@@ -144,6 +144,7 @@
c(auto_qmail,"bin","except",auto_uido,auto_gidq,0755);
c(auto_qmail,"bin","maildirmake",auto_uido,auto_gidq,0755);
c(auto_qmail,"bin","maildir2mbox",auto_uido,auto_gidq,0755);
+ c(auto_qmail,"bin","update_tmprsadh",auto_uido,auto_gidq,0755);
c(auto_qmail,"man/man5","addresses.5",auto_uido,auto_gidq,0644);
c(auto_qmail,"man/cat5","addresses.0",auto_uido,auto_gidq,0644);
big-todo: failed hunks
qmail-send.c:
@@ -1255,8 +1256,7 @@
char ch;
int match;
unsigned long id;
- unsigned int len;
- direntry *d;
+ int z;
int c;
unsigned long uid;
unsigned long pid;
@@ -1267,32 +1267,26 @@
if (flagexitasap) return;
- if (!tododir)
+ if (!flagtododir)
{
if (!trigger_pulled(rfds))
if (recent < nexttodorun)
return;
trigger_set();
- tododir = opendir("todo");
- if (!tododir)
- {
- pausedir("todo");
- return;
- }
+ readsubdir_init(&todosubdir, "todo", pausedir);
+ flagtododir = 1;
nexttodorun = recent + SLEEP_TODO;
}
- d = readdir(tododir);
- if (!d)
+ switch(readsubdir_next(&todosubdir, &id))
{
- closedir(tododir);
- tododir = 0;
- return;
+ case 1:
+ break;
+ case 0:
+ flagtododir = 0;
+ default:
+ return;
}
- if (str_equal(d->d_name,".")) return;
- if (str_equal(d->d_name,"..")) return;
- len = scan_ulong(d->d_name,&id);
- if (!len || d->d_name[len]) return;
fnmake_todo(id);
I'm extremely happy (@josuah, I don't have the words, but at least "thank you") to find out immediately that this would break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First sweep of things, not meant to be exhaustive.
I agree that I do still worry about
As usual, I’m not quibbling about what needs doing, only about the ordering. My thinking on this, right now, is:
Post-1.09, I’m all for writing tests around |
I would squash the second commit into the first, and note what you have done in the commit message. That way these files don't clutter the git history forever. |
And one other thing that I just found: the first commit should really just be authored by Bruce. Since @schmonz did the actual commit he will show up in history as the one who introduced that code into the repo. End of story (Co-authors could be used for people helping write the original codebase). People that helped with this import should get co-author status for the following cleanups. |
f06c05c
to
b98f098
Compare
I've rewritten the PR so it consists of
Since the code is very nearly identical to what many of us have been running in production for many years, I'm assuming we're comfortable deferring further cleanups to the code, all of which I've noted locally for later ("later" == after 1.09 and after we have automated tests). Does anyone have any doubts or concerns on the license front? Or could this be mergeable? |
202c30d
to
06d121d
Compare
Updates:
Assuming Bruce can relicense qmail-qfilter without too much delay, I'll wait for that, then update this PR to remove the GPLv2 bits and catch up with the latest version. |
f791a11
to
03c3cfd
Compare
From qmail-qfilter.git at commit 795718fe819266df3a363519231f513a8e882f9e (the newest available on master).
Co-authored-by: Amitai Schleier <[email protected]> Co-authored-by: Astrid Sawatsky <[email protected]> Co-authored-by: Evan Theriault <[email protected]> Co-authored-by: Jacqueline Bilston <[email protected]> Co-authored-by: Joel Samaroo <[email protected]> Co-authored-by: Patrick Ledzian <[email protected]> Co-authored-by: Trung Vo <[email protected]> Co-authored-by: Yevhen Viktorov <[email protected]>
7015b72
to
0603868
Compare
This has been done 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should put those things "marked for future cleanups", at least the missing range checks and such, right into this branch. Or we flood @bruceg with PRs cleaning up all these and then just merge the latest version. Whatever works best ;)
{ | ||
const char* env; | ||
size_t offset; | ||
if ((env = mmap(0, env_len, PROT_READ, MAP_PRIVATE, ENVIN, 0)) == MAP_FAILED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first 0 must become NULL
.
static size_t msg_len = 0; | ||
|
||
/* Parse the sender address into user and host portions */ | ||
size_t parse_sender(const char* env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole functions has a fundamental issue if the input is not "sane", i.e. if the data that is mmap'ed is not 0-terminated. In this case the strlen()
below will run out of bounds and likely crash. The length passed to mmap will need to be passed to this function, and to parse_rcpts()
as well so one can replace the strlen(env)
by memchr(env, 0, envlen)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to address this upstream in bruceg/qmail-qfilter#4
/* Create a temporary invisible file opened for read/write */ | ||
int mktmpfile() | ||
{ | ||
char filename[sizeof(TMPDIR)+19] = TMPDIR "/fixheaders.XXXXXX"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of the buffer is not needed, when it is initialized it will automatically get the correct size. This will as well remove the magic 19.
command* tail = 0; | ||
command* head = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be NULL
.
|
||
filters = parse_args(argc-1, argv+1); | ||
|
||
if ((qqargv[0] = getenv("QQF_QMAILQUEUE")) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NULL
One of the blockers for releasing notqmail 1.09 is that in 1.08 we introduced a regression in how recipient addresses are qualified. We probably could have avoided the mistake if there'd been tests around that code, and before we try to fix it we'll want those tests in place.
With the in-tree tools currently available, there's no way to test the recipient-address qualification code without also creating and manipulating an on-disk mail queue. We'd like to avoid that integration point, and we can accomplish this by importing qmail-qfilter.
Besides its utility in tests,
qmail-qfilter
has been a staple of many qmail administrators' production environments for well over a decade, and its design (along with the QMAILQUEUE patch we've had since 1.07) feels like something qmail should have always included. In conjunction with the custom-error patch already merged,qmail-qfilter
will finally provide our users with a no-patching-required Unix-filter API for programmatically modifying or rejecting messages during the SMTP conversation.Besides its utility in production, having
qmail-qfilter
in-tree provides a needed building block for future functionality.