DragonFlyBSD

Daniel Roethlisberger wrote:

When delivering mail to multiple recipients, dma(8) will fork for
each recipient and attempt parallel delivery. Separate delivery
processes for multiple recipients of the same message all use the
same FILE* on the same file descriptor. If a process is loosing
the CPU during the delivery process, another delivery process for
the same message will wreak havoc on the file descriptor. The
symptom is dma(8) logging a failed delivery and bouncing the
message for some or all of the recipients:

Jul 3 20:57:21 marvin dma³²⁶⁴⁰: 15c.284010d0: remote delivery
failed:corrupted queue file

Thanks for your submission! I've taken a different approach, could you please test whether it works for you? (Untested patch).

thanks,
simon

Simon 'corecode' Schuber <corecode@fs.ei.tum.de> 2009-07-09:

Daniel Roethlisberger wrote:

When delivering mail to multiple recipients, dma(8) will fork for
each recipient and attempt parallel delivery. Separate delivery
processes for multiple recipients of the same message all use the
same FILE* on the same file descriptor. If a process is loosing
the CPU during the delivery process, another delivery process for
the same message will wreak havoc on the file descriptor. The
symptom is dma(8) logging a failed delivery and bouncing the
message for some or all of the recipients:

Jul 3 20:57:21 marvin dma³²⁶⁴⁰: 15c.284010d0: remote delivery
failed:corrupted queue file

Thanks for your submission! I've taken a different approach, could you
please test whether it works for you? (Untested patch).

thanks,
simon

This patch doesn't work for me: dma delivers fine to the first
recipient, but fails delivery for all other recipients, logging
an illegal seek to syslog:

Jul 10 01:15:52 marvin dma⁴⁵¹³⁷: e7.28401130: remote delivery
deferred: cannot seek: Illegal seek

Daniel Roethlisberger wrote:

Simon 'corecode' Schuber <corecode@fs.ei.tum.de> 2009-07-09:

--- a/libexec/dma/dma.c
+++ b/libexec/dma/dma.c
@ -484,6 +484,7 @ go_background(struct queue *queue) {
struct sigaction sa;
struct qitem *it;
+ FILE *newqf;
pid_t pid;

if (daemonize && daemon(0, 0) != 0) {
@ -515,6 +516,17 @ go_background(struct queue *queue) *

• return and deliver mail
  /
  + /
  + * We have to prevent sharing of fds between children, so
  + * we have to dup the queue fd.
  + */
  + newqf = fdopen(fileno(it->queuef), "r");

Actually, fdopen() will not dup() the file descriptors, just
create a new FILE* handle operating on the same fd.

Uh. I ment to add a dup() there, but I moved a lot of code, so I
forgot... What about the attached patch?

cheers
simon

Simon 'corecode' Schubert wrote:

Uh. I ment to add a dup() there, but I moved a lot of code, so I
forgot... What about the attached patch?

And of course I attached the wrong one. Here the right one.

[...]

--- a/libexec/dma/dma.c
+++ b/libexec/dma/dma.c
@ -484,6 +484,7 @ go_background(struct queue *queue) {
struct sigaction sa;
struct qitem *it;
+ FILE *newqf;
pid_t pid;

if (daemonize && daemon(0, 0) != 0) {
@ -515,6 +516,17 @ go_background(struct queue *queue) *

• return and deliver mail
  /
  + /
  + * We have to prevent sharing of fds between children, so
  + * we have to dup the queue fd.
  + */
  + newqf = fdopen(dup(fileno(it->queuef)), "r");
  + if (newqf == NULL) {
  + syslog(LOG_ERR, "can not dup queue fd: %m");
  + exit(1);
  + }
  + fclose(it->queuef);
  + it->queuef = newqf;
  return (it);

default:

This is still not sufficient (logs below; of 3 recipients, 2 were
successful but with corrupted contents, 1 failed due to queue
file corruption).

To go down that route, I think you'd need to fully reopen the
file starting with open(). This opens another can of worms:
unlink() races. I guess it is easier to lock the queue file
instead of fully reopening and fixing the unlink() race. But
maybe I'm missing something.

Jul 10 10:41:30 marvin dma⁵²⁹⁵⁰: d6.28401130: mail from=<roe@marvin.ustcor.roe.ch> to=<daniel+3@roe.ch>
Jul 10 10:41:30 marvin dma⁵²⁹⁵¹: d6.28401100: mail from=<roe@marvin.ustcor.roe.ch> to=<daniel+2@roe.ch>
Jul 10 10:41:30 marvin dma⁵²⁹⁵¹: d6.28401100: trying delivery
Jul 10 10:41:30 marvin dma⁵²⁹⁵¹: d6.28401100: using smarthost (calvin.ustdmz.roe.ch:587)
Jul 10 10:41:30 marvin dma⁵²⁹⁴⁹: d6.284010d0: mail from=<roe@marvin.ustcor.roe.ch> to=<daniel+1@roe.ch>
Jul 10 10:41:30 marvin dma⁵²⁹⁴⁹: d6.284010d0: trying delivery
Jul 10 10:41:30 marvin dma⁵²⁹⁴⁹: d6.284010d0: using smarthost (calvin.ustdmz.roe.ch:587)
Jul 10 10:41:30 marvin dma⁵²⁹⁵⁰: d6.28401130: trying delivery
Jul 10 10:41:30 marvin dma⁵²⁹⁵⁰: d6.28401130: using smarthost (calvin.ustdmz.roe.ch:587)
Jul 10 10:41:34 marvin dma⁵²⁹⁵¹: d6.28401100: SSL initialization successful
Jul 10 10:41:34 marvin dma⁵²⁹⁵¹: d6.28401100: Use SMTP authentication
Jul 10 10:41:37 marvin dma⁵²⁹⁵⁰: d6.28401130: SSL initialization successful
Jul 10 10:41:37 marvin dma⁵²⁹⁴⁹: d6.284010d0: SSL initialization successful
Jul 10 10:41:37 marvin dma⁵²⁹⁵⁰: d6.28401130: Use SMTP authentication
Jul 10 10:41:37 marvin dma⁵²⁹⁴⁹: d6.284010d0: Use SMTP authentication
Jul 10 10:41:45 marvin dma⁵²⁹⁵¹: d6.28401100: delivery successful
Jul 10 10:41:48 marvin dma⁵²⁹⁴⁹: d6.284010d0: remote delivery failed:corrupted queue file
Jul 10 10:41:48 marvin dma⁵²⁹⁴⁹: d6.284010d0: delivery failed, bouncing
Jul 10 10:41:48 marvin dma⁵²⁹⁴⁹: d8.28401250: mail from=<> to=<roe>
Jul 10 10:41:48 marvin dma⁵²⁹⁴⁹: d8.28401250: trying delivery
Jul 10 10:41:48 marvin dma⁵²⁹⁴⁹: d8.28401250: delivery successful
Jul 10 10:41:57 marvin dma⁵²⁹⁵⁰: d6.28401130: delivery successful

Matthew Dillon wrote:

dup() doesn't solve the problem. A dup()'d descriptor still
points to a common file pointer and thus a common seek position.

Only open() will generate a new file pointer backing the file
descriptor.

That's not true.

cheers
simon

Simon 'corecode' Schubert wrote:

Matthew Dillon wrote:

dup() doesn't solve the problem. A dup()'d descriptor still
points to a common file pointer and thus a common seek position.

Only open() will generate a new file pointer backing the file
descriptor.

That's not true.

Nevermind, you're right. I'm too close to sleeping times, and stdio is
doing some heavy buffering.

Daniel Roethlisberger <daniel@roe.ch> 2009-07-10:

I guess it is easier to lock the queue file instead of fully
reopening and fixing the unlink() race.

I cleaned up my original hack, maybe this version is a tad more
convincing :)

:Uh. I ment to add a dup() there, but I moved a lot of code, so I
:forgot... What about the attached patch?
:
:cheers
: simon

dup() doesn't solve the problem.  A dup()'d descriptor still
    points to a common file pointer and thus a common seek position.

Only open() will generate a new file pointer backing the file
    descriptor.

-Matt

Daniel Roethlisberger wrote:

Daniel Roethlisberger <daniel@roe.ch> 2009-07-10:

I guess it is easier to lock the queue file instead of fully
reopening and fixing the unlink() race.

I cleaned up my original hack, maybe this version is a tad more
convincing :)

This doesn't work for me. All processes are stuck in state lockf. I
think we can't reliably combine fcntl locks and flock locks.

cheers
simon

Simon 'corecode' Schuber <submit@crater.dragonflybsd.org> 2009-07-16:

Simon 'corecode' Schubert <corecode@fs.ei.tum.de> added the comment:

Daniel Roethlisberger wrote:

Daniel Roethlisberger <daniel@roe.ch> 2009-07-10:

I guess it is easier to lock the queue file instead of fully
reopening and fixing the unlink() race.

I cleaned up my original hack, maybe this version is a tad more
convincing :)

This doesn't work for me. All processes are stuck in state lockf. I
think we can't reliably combine fcntl locks and flock locks.

Is that a DragonFlyBSD issue? On FreeBSD seem to be compatible
(both according to manual page and practical testing). Using
flock to synchronize on the dup()d / fork()d queue file
descriptor doesnŽt work in this case due to the semantics of
flock.

In any case, IŽm sorry that I cannot test / respond again for 3
weeks since IŽm abroad, far away from keyboard.

Daniel Roethlisberger wrote:

Simon 'corecode' Schuber <submit@crater.dragonflybsd.org> 2009-07-16:

Simon 'corecode' Schubert <corecode@fs.ei.tum.de> added the comment:

Daniel Roethlisberger wrote:

Daniel Roethlisberger <daniel@roe.ch> 2009-07-10:

I guess it is easier to lock the queue file instead of fully
reopening and fixing the unlink() race.

I cleaned up my original hack, maybe this version is a tad more
convincing :)

This doesn't work for me. All processes are stuck in state lockf. I
think we can't reliably combine fcntl locks and flock locks.

Is that a DragonFlyBSD issue? On FreeBSD seem to be compatible
(both according to manual page and practical testing). Using
flock to synchronize on the dup()d / fork()d queue file
descriptor doesnŽt work in this case due to the semantics of
flock.

In any case, IŽm sorry that I cannot test / respond again for 3
weeks since IŽm abroad, far away from keyboard.

No problem, I fixed the issue by re-opening the queue file. Thanks for
your bug report!

cheers
simon

flock locks are effective lockf locks but cover the ENTIRE range.
It is not a good idea to try to combine them. Use one or the other.

lockf locks (via fcntl) follow POSIX semanics, I believe, which means
    that when you close() a descriptor, ANY LOCK HELD BY THAT PROCESS ON
    THAT FILE will also be released, even if it was obtained by another 
    descriptor.  This is badly broken but its what POSIX says and what we
    have to do.

If you are fork()ing you want to be sure that you only have one file
    descriptor open for the file that you intend to maintain a lock on.
    If you open/close the file within that same process the original lock
    on the still-open descriptor will be lost.  For fcntl-based lockf locks.

I believe flock() behaves more rationally.

-Matt

Matthew Dillon wrote:

I believe flock() behaves more rationally.

Yah, I simply went and re-opened the file instead of using locks. I also
have a new queueing code in the works that will get rid of this problem
entirely.

cheers
simon

:Matthew Dillon wrote:
:> I believe flock() behaves more rationally.
:
:Yah, I simply went and re-opened the file instead of using locks. I also
:have a new queueing code in the works that will get rid of this problem
:entirely.
:
:cheers
: simon

The last time I looked at the code I also noticed that the file
    descriptors were getting spammed by the fork()... that the children
    would inherit the parent's entire fd set, even open fd's related
    to queue files other then the one the child is supposed to
    process.

-Matt

Matthew Dillon wrote:

:Matthew Dillon wrote:
:> I believe flock() behaves more rationally.
:
:Yah, I simply went and re-opened the file instead of using locks. I also
:have a new queueing code in the works that will get rid of this problem
:entirely.
:
:cheers
: simon

The last time I looked at the code I also noticed that the file
descriptors were getting spammed by the fork()... that the children
would inherit the parent's entire fd set, even open fd's related
to queue files other then the one the child is supposed to
process.

Yes, I'm working on all that.

cheers
simon