[9front] [Bug] Mail fails to send attachments

[9front] [Bug] Mail fails to send attachments

[theinicke]

Today I have noticed that since the rewrite Mail fails to send attachments.
That is if one includes an "Attach: /some/file" line and posts it,
(if file exists) marshal suicides like so: 66286: marshal 66286: sys: write on closed pipe pc=0x25e04
leaving us with a sent mail without the attachments.

I have not been able to track down the cause yet.
Have reproduced it on a clean install updated to latest HEAD
to make sure it is not caused by any of my modifications.
Here are the symptoms:

If in /sys/src/cmd/upas/marshal/marshal.c:421 attachment function is called, then
in /sys/src/cmd/upas/marshal/marshal.c:430 the error occurs.

--
Tobias Heinicke

Re: [9front] [Bug] Mail fails to send attachments

[sirjofri]

19.02.2021 20:56:00 theinicke@bss-wf.de:
> Today I have noticed that since the rewrite Mail fails to send 
> attachments.
> That is if one includes an "Attach: /some/file" line and posts it,
> (if file exists) marshal suicides like so: 66286: marshal 66286: sys: 
> write on closed pipe pc=0x25e04
> leaving us with a sent mail without the attachments.
>
> I have not been able to track down the cause yet.
> Have reproduced it on a clean install updated to latest HEAD
> to make sure it is not caused by any of my modifications.
> Here are the symptoms:
>
> If in /sys/src/cmd/upas/marshal/marshal.c:421 attachment function is 
> called, then
> in /sys/src/cmd/upas/marshal/marshal.c:430 the error occurs.

Same here. The same happens when using Include: instead of Attach. Ori is 
unable to reproduce this, afaik. Using marshal -8 directly works fine 
btw.

sirjofri

Re: [9front] [Bug] Mail fails to send attachments

[Romano]

I don't get a suicide but noticed last night that Nail's comp.c adds content-type and content-enconding (IIRC; I'm in the desert right now). I removed those from the comp.c and added to my /mail/box/$user/headers and that resolved the issue, but there are still two content-type and content-encoding for the sent email, based on on the rawheader file. I haven't been able to test more--perhaps the marshal doc isn't correct re: adding headers from the headers file when none exist, and just always adds them.

On February 19, 2021 8:13:36 PM UTC, sirjofri <sirjofri+ml-9front@sirjofri.de> wrote:
>
>19.02.2021 20:56:00 theinicke@bss-wf.de:
>> Today I have noticed that since the rewrite Mail fails to send 
>> attachments.
>> That is if one includes an "Attach: /some/file" line and posts it,
>> (if file exists) marshal suicides like so: 66286: marshal 66286: sys:
>
>> write on closed pipe pc=0x25e04
>> leaving us with a sent mail without the attachments.
>>
>> I have not been able to track down the cause yet.
>> Have reproduced it on a clean install updated to latest HEAD
>> to make sure it is not caused by any of my modifications.
>> Here are the symptoms:
>>
>> If in /sys/src/cmd/upas/marshal/marshal.c:421 attachment function is 
>> called, then
>> in /sys/src/cmd/upas/marshal/marshal.c:430 the error occurs.
>
>Same here. The same happens when using Include: instead of Attach. Ori
>is 
>unable to reproduce this, afaik. Using marshal -8 directly works fine 
>btw.
>
>sirjofri

Re: [9front] [Bug] Mail fails to send attachments

[ori]

Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:
> 
> 19.02.2021 20:56:00 theinicke@bss-wf.de:
> > Today I have noticed that since the rewrite Mail fails to send 
> > attachments.
> > That is if one includes an "Attach: /some/file" line and posts it,
> > (if file exists) marshal suicides like so: 66286: marshal 66286: sys: 
> > write on closed pipe pc=0x25e04
> > leaving us with a sent mail without the attachments.
> >
> > I have not been able to track down the cause yet.
> > Have reproduced it on a clean install updated to latest HEAD
> > to make sure it is not caused by any of my modifications.
> > Here are the symptoms:
> >
> > If in /sys/src/cmd/upas/marshal/marshal.c:421 attachment function is 
> > called, then
> > in /sys/src/cmd/upas/marshal/marshal.c:430 the error occurs.
> 
> Same here. The same happens when using Include: instead of Attach. Ori is 
> unable to reproduce this, afaik. Using marshal -8 directly works fine 
> btw.
> 
> sirjofri
> 

I don't know why marshal -8 would work, when
Mail wouldn't. We just invoke:

	upas/marshal -8 -S outgoing

The pipe being closed is talking to upas/send,
which is likely dying or exiting early. This
may help capture it:

diff -r 39b061370f36 sys/src/cmd/upas/marshal/marshal.c
--- a/sys/src/cmd/upas/marshal/marshal.c	Wed Feb 17 11:20:13 2021 +0100
+++ b/sys/src/cmd/upas/marshal/marshal.c	Fri Feb 19 13:08:55 2021 -0800
@@ -1172,6 +1172,7 @@
 
 	err = nil;
 	while((w = wait()) != nil){
+		fprint(2, "%d: %s\n", w->pid, w->msg);
 		if(w->pid == pid || w->pid == pgppid)
 			if(w->msg[0] != 0)
 				err = estrdup(w->msg);


If you have dtracy enabled, it may be possible
to snoop the exit status, which could help
get an idea of what's going on. Don't see a
great way to get the process name, so it'll
take some guesswork:

	dtracy 'sys:exits:entry if(arg0 != 0) {
		print probe, pid, (string)arg0
	}'

Re: [9front] [Bug] Mail fails to send attachments

[sirjofri]

Hello,

I did some testing and this is what I found out:

upas/send does not fail. This is at least from what I found out using 
dtracy (no status error) and other tests. It seems only marshal fails.

Marshal doesn't always call send. If there's a pipefrom, it executes that 
instead. I tried using a minimal pipefrom (saving args and all stdin in 
file, to verify) and marshal still fails (I verified that send isn't 
called at all).

This could mean that the pipe is closed by marshal itself (by one of its 
child processes).

Marshal does fork pretty often. It even forks inside forks. I'll try to 
find out more about that tomorrow. I can imagine there's one child that 
basically closes the pipe when another child still wants to write.

I currently assume this happens somewhere in marshal.c:/^sendmail, but 
today I'm way to tired to find it.

I'll be available again tomorrow and hopefully we can find it.

sirjofri

Re: [9front] [Bug] Mail fails to send attachments

[sirjofri]

I noticed that marshal also handles the whole boundary stuff and 
attachments. The output file I get in pipefrom does not contain the data 
from the attachment.

To verify I made this pipefrom:

#!/bin/rc
echo $* > /tmp/msgargs
cat > /tmp/msgcontent

Then look at these files.

The whole thing is formatted as a multipart message, but it only contains 
the message text, no attachments.

I also looked at the history of the marshal code, I don't think the 
sendmail function is bad here.

sirjofri

Re: [9front] [Bug] Mail fails to send attachments

[ori]

Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:
> I noticed that marshal also handles the whole boundary stuff and 
> attachments. The output file I get in pipefrom does not contain the data 
> from the attachment.
> 
> To verify I made this pipefrom:
> 
> #!/bin/rc
> echo $* > /tmp/msgargs
> cat > /tmp/msgcontent
> 
> Then look at these files.
> 
> The whole thing is formatted as a multipart message, but it only contains 
> the message text, no attachments.
> 
> I also looked at the history of the marshal code, I don't think the 
> sendmail function is bad here.
> 
> sirjofri
> 

I'd look at whether we're closing the wrong fd
if we're passing '-S'.

Re: [9front] [Bug] Mail fails to send attachments

[sirjofri]

Hello,

20.02.2021 01:49:09 ori@eigenstate.org:
> Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:
>> I noticed that marshal also handles the whole boundary stuff and
>> attachments. The output file I get in pipefrom does not contain the 
>> data
>> from the attachment.
>>
>> To verify I made this pipefrom:
>>
>> #!/bin/rc
>> echo $* > /tmp/msgargs
>> cat > /tmp/msgcontent
>>
>> Then look at these files.
>>
>> The whole thing is formatted as a multipart message, but it only 
>> contains
>> the message text, no attachments.
>>
>> I also looked at the history of the marshal code, I don't think the
>> sendmail function is bad here.
>>
>> sirjofri
>>
>
> I'd look at whether we're closing the wrong fd
> if we're passing '-S'.

I found out, it's something completely different.

The function foldername builds the path to the outgoing directory. For 
this, it uses the upasname environment variable (or the login name if 
it's not set).

However, I had my upasname set to my sender address (joel@sirjofri.de), 
so it builds the string: /mail/box/joel@sirjofri.de/outgoing, which of 
course cannot be opened (it doesn't exist!).

Therefore, openfolder does return a wrong Biobuf with a file descriptor 
-1, our tee function fails to write there and nothing works.

Removing my upasname variable confirmed this bug.

Now I just need to find a proper solution to this. Looking at the 
returned file descriptor could be one, writing not to user but login 
directory another.

If you want to follow my description in code, marshal.c:/^sendmail, 
switch() case 0:, if (rcvr != nil), switch() case 0, the foldername 
function call.

sirjofri

Re: [9front] [Bug] Mail fails to send attachments

[ori]

Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:
> 
> 19.02.2021 20:56:00 theinicke@bss-wf.de:
> > Today I have noticed that since the rewrite Mail fails to send 
> > attachments.
> > That is if one includes an "Attach: /some/file" line and posts it,
> > (if file exists) marshal suicides like so: 66286: marshal 66286: sys: 
> > write on closed pipe pc=0x25e04
> > leaving us with a sent mail without the attachments.
> >
> > I have not been able to track down the cause yet.
> > Have reproduced it on a clean install updated to latest HEAD
> > to make sure it is not caused by any of my modifications.
> > Here are the symptoms:
> >
> > If in /sys/src/cmd/upas/marshal/marshal.c:421 attachment function is 
> > called, then
> > in /sys/src/cmd/upas/marshal/marshal.c:430 the error occurs.
> 
> Same here. The same happens when using Include: instead of Attach. Ori is 
> unable to reproduce this, afaik. Using marshal -8 directly works fine 
> btw.
> 
> sirjofri
> 

So, after some debugging with sirjofri, it looks
like this may have something to do with the '-S'
flag that we're passing to upas/marshal.

If you can confirm by running with 'Mail -O' to
disable saving outgoing messages, that would be
appreciated. It also gives a hint for where to
debug.

Re: [9front] [Patch] Mail fails to send attachments

[sirjofri]

This is a multi-part message in MIME format.
--upas-kbnxydoadswlscajpsgakobprj
Content-Disposition: inline
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

Save outgoing files in user mailbox, fixes closed pipe bug

I noticed it is no closed pipe, but the constructed foldername for the
save location was built using the upasname variable, which can be set
to an alias.

This patch fixes that and uses the logged in username instead.
Therefore it opens the file correctly and the write works.

For example, before:

	upasname: joel@sirjofri.de
	foldername: /mail/box/joel@sirjofri.de/outgoing

after:

	upasname: joel@sirjofri.de
	foldername: /mail/box/sirjofri/outgoing

If an attachment is here, then the patch works (on my machine, at
least).

We should still check the returned file descriptor, just in case the
user has no mailbox directory.

sirjofri

.
--upas-kbnxydoadswlscajpsgakobprj
Content-Disposition: inline
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

diff -r 297026d9dc30 sys/src/cmd/upas/marshal/marshal.c
--- a/sys/src/cmd/upas/marshal/marshal.c	Thu Feb 18 21:40:30 2021 +0100
+++ b/sys/src/cmd/upas/marshal/marshal.c	Sat Feb 20 14:09:26 2021 +0100
@@ -1086,7 +1086,7 @@
 			case 0:
 				close(pfd[0]);
 				/* BOTCH; "From " time gets changed */
-				b = openfolder(foldername(nil, user, rcvr), time(0));
+				b = openfolder(foldername(nil, login, rcvr), time(0));
 				fd = b? Bfildes(b): -1;
 				printunixfrom(fd);
 				tee(0, pfd[1], fd);
--upas-kbnxydoadswlscajpsgakobprj--

Re: [9front] [Patch] Mail fails to send attachments

[hiro]

gmail still displays your mails weirdly, might be unrelated but not sure:

In-Reply-To: <E07C376E4275255C9E2473CC13E5C94B@eigenstate.org>
MIME-Version: 1.0
Content-Type: multipart/mixed;
boundary="upas-kbnxydoadswlscajpsgakobprj" <<< this seems to be
ignored by my gmail that displays the message. is it too early?
shouldn't it be the last line in the headers?
List-ID: <9front.9front.org> <<< obviously this got added by 9front
mailing list... might be a harmful interaction with your header... ?
List-Help: <http://lists.9front.org>
X-Glyph: ➈
X-Bullshit: encrypted interface-aware base high-performance frontend
Subject: Re: [9front] [Patch] Mail fails to send attachments
Reply-To: 9front@9front.org
Precedence: bulk
<< this empty line marks the beginning of the message...
This is a multi-part message in MIME format. << ...which is why gmail
displays this
--upas-kbnxydoadswlscajpsgakobprj <<<and ignores this, displaying this
and the rest of the body as plain text including those headers.
Content-Disposition: inline
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit


On 2/20/21, sirjofri <sirjofri+ml-9front@sirjofri.de> wrote:
> This is a multi-part message in MIME format.
> --upas-kbnxydoadswlscajpsgakobprj
> Content-Disposition: inline
> Content-Type: text/plain; charset="US-ASCII"
> Content-Transfer-Encoding: 7bit
>
> Save outgoing files in user mailbox, fixes closed pipe bug
>
> I noticed it is no closed pipe, but the constructed foldername for the
> save location was built using the upasname variable, which can be set
> to an alias.
>
> This patch fixes that and uses the logged in username instead.
> Therefore it opens the file correctly and the write works.
>
> For example, before:
>
> 	upasname: joel@sirjofri.de
> 	foldername: /mail/box/joel@sirjofri.de/outgoing
>
> after:
>
> 	upasname: joel@sirjofri.de
> 	foldername: /mail/box/sirjofri/outgoing
>
> If an attachment is here, then the patch works (on my machine, at
> least).
>
> We should still check the returned file descriptor, just in case the
> user has no mailbox directory.
>
> sirjofri
>
> .
> --upas-kbnxydoadswlscajpsgakobprj
> Content-Disposition: inline
> Content-Type: text/plain; charset="US-ASCII"
> Content-Transfer-Encoding: 7bit
>
> diff -r 297026d9dc30 sys/src/cmd/upas/marshal/marshal.c
> --- a/sys/src/cmd/upas/marshal/marshal.c	Thu Feb 18 21:40:30 2021 +0100
> +++ b/sys/src/cmd/upas/marshal/marshal.c	Sat Feb 20 14:09:26 2021 +0100
> @@ -1086,7 +1086,7 @@
>  			case 0:
>  				close(pfd[0]);
>  				/* BOTCH; "From " time gets changed */
> -				b = openfolder(foldername(nil, user, rcvr), time(0));
> +				b = openfolder(foldername(nil, login, rcvr), time(0));
>  				fd = b? Bfildes(b): -1;
>  				printunixfrom(fd);
>  				tee(0, pfd[1], fd);
> --upas-kbnxydoadswlscajpsgakobprj--
>

Re: [9front] bad headers in mail (was: Mail attachment bug)

[sirjofri]

20.02.2021 14:37:42 hiro <23hiro@gmail.com>:
> gmail still displays your mails weirdly, might be unrelated but not 
> sure:

Hey hiro,

I sent mails to myself and looked at the headers, also at what you sent.

If the multipart header must be the last then my sent mail is correct (at 
least from what I can find out/what upas/send receives from marshal). I 
can imagine it's the mailing list adding headers in weird places?

It also might be different using Include: and Attach:. Can I send you two 
mails, one with Include:d attachment, the other Attach:ed?

sirjofri

Re: [9front] bad headers in mail (was: Mail attachment bug)

[hiro]

tbh i only skimmed the RFC, and the wrong one at that.
i have seen another one now that does the same thing where the
multipart header is also not the last line in the headers, and it gets
displayed properly.

you can send me those mails, yeah.

it might be that your mail is actually compliant, but in a rare form
that gmail ignores.
i can not make sense of it tbh.

maybe the in-line stuff forces gmail to show them as is and with the headers?

On 2/20/21, sirjofri <sirjofri+ml-9front@sirjofri.de> wrote:
>
> 20.02.2021 14:37:42 hiro <23hiro@gmail.com>:
>> gmail still displays your mails weirdly, might be unrelated but not
>> sure:
>
> Hey hiro,
>
> I sent mails to myself and looked at the headers, also at what you sent.
>
> If the multipart header must be the last then my sent mail is correct (at
> least from what I can find out/what upas/send receives from marshal). I
> can imagine it's the mailing list adding headers in weird places?
>
> It also might be different using Include: and Attach:. Can I send you two
> mails, one with Include:d attachment, the other Attach:ed?
>
> sirjofri
>

Re: [9front] [Patch] Mail fails to send attachments

[sirjofri]

Somehow this mail wasn't sent. Here it is:

20.02.2021 14:29:59 joel@sirjofri.de:

Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:

> We should still check the returned file descriptor, just in case the
> user has no mailbox directory.

This patch includes that check.

I adjusted the tee function so that the second destination is ignored
if it's invalid.

sirjofri

.
diff -r 297026d9dc30 sys/src/cmd/upas/marshal/marshal.c
--- a/sys/src/cmd/upas/marshal/marshal.c  Thu Feb 18 21:40:30 2021 +0100
+++ b/sys/src/cmd/upas/marshal/marshal.c  Sat Feb 20 14:27:35 2021 +0100
@@ -985,10 +985,12 @@
  int n;
  char buf[8*1024];

- while ((n = read(in, buf, sizeof buf)) > 0)
-   if (write(out1, buf, n) != n ||
-       write(out2, buf, n) != n)
+ while ((n = read(in, buf, sizeof buf)) > 0){
+   if (write(out1, buf, n) != n)
      break;
+   if (out2 >= 0 && write(out2, buf, n) != n)
+     break;
+ }
}

/* print the unix from line */
@@ -1086,11 +1088,14 @@
      case 0:
        close(pfd[0]);
        /* BOTCH; "From " time gets changed */
-       b = openfolder(foldername(nil, user, rcvr), time(0));
-       fd = b? Bfildes(b): -1;
-       printunixfrom(fd);
-       tee(0, pfd[1], fd);
-       write(fd, "\n", 1);
+       b = openfolder(foldername(nil, login, rcvr), time(0));
+       if (b){
+         fd = Bfildes(b);
+         printunixfrom(fd);
+         tee(0, pfd[1], fd);
+         write(fd, "\n", 1);
+       } else
+         tee(0, pfd[1], -1);
        closefolder(b);
        exits(0);
      default:

Re: [9front] [Patch] Mail fails to send attachments

[ori]

Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:
> Somehow this mail wasn't sent. Here it is:
> 
> 20.02.2021 14:29:59 joel@sirjofri.de:
> 
> Quoth sirjofri <sirjofri+ml-9front@sirjofri.de>:
> 
> > We should still check the returned file descriptor, just in case the
> > user has no mailbox directory.
> 
> This patch includes that check.
> 
> I adjusted the tee function so that the second destination is ignored
> if it's invalid.
>

Committed with some nitpicky style tweaks. Thanks.