Bug 126263 - sending emails with attachments via "com.sun.star.system.SimpleSystemMail" can cause data loss
Summary: sending emails with attachments via "com.sun.star.system.SimpleSystemMail" ca...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (show other bugs)
Version:
(earliest affected)
6.1.6.3 release
Hardware: All Windows (All)
: medium normal
Assignee: Mike Kaganski
URL:
Whiteboard: target:7.5.0 target:7.4.0.0.beta2 tar...
Keywords:
Depends on:
Blocks: File-Send
  Show dependency treegraph
 
Reported: 2019-07-07 08:19 UTC by Oliver Brinzing
Modified: 2022-07-28 13:38 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
mail demo basic (1.25 KB, text/plain)
2019-07-07 08:19 UTC, Oliver Brinzing
Details
calling senddoc.exe (81.00 KB, image/png)
2019-07-07 08:20 UTC, Oliver Brinzing
Details
senddoc.exe finished (59.63 KB, image/png)
2019-07-07 08:21 UTC, Oliver Brinzing
Details
deleted attachment (118.59 KB, application/pdf)
2019-08-01 17:11 UTC, Oliver Brinzing
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Brinzing 2019-07-07 08:19:26 UTC
Created attachment 152612 [details]
mail demo basic

sending emails with attachments via "com.sun.star.system.SimpleSystemMail" can cause data loss, if file paths instead of file urls are added.

> void setAttachement([in] sequence< string > aAttachement) 	
>      raises (::com::sun::star::lang::IllegalArgumentException)	
> aAttachement Sets a sequence of file URLs specifying the files that should
> be attached to the mail. The given file URLs must conform to Rfc1738. 
> The method does not check if the specified file or files really exist. 

steps to reproduce:
- add some pdf files: a.pdf, b.pdf, c.pdf, d.pdf to LO working directory
- run attached macro (-> mail_demo.bas)
-> b.pdf is removed, cause it is added as file path

root cause:
"senddoc.exe" tries to copy attachments to a temp dir before calling email client. these files are removed on exit (). but if copy fails (e.g. attachment is a file path instead of a file url) it uses the original file path.

expectation:
either setAttachment() throws an exception if a file path (or invalid url) is added or "senddoc.exe" does not remove original file.

smplmailclient.cxx:

OUString CSmplMailClient::CopyAttachment(const OUString& sOrigAttachURL, OUString& sUserVisibleName)
{
    // We do two things here:
    // 1. Make the attachment temporary filename to not contain any fancy characters possible in
    // original filename, that could confuse mailer, and extract the original filename to explicitly
    // define it;
    // 2. Allow the copied files be outside of the session's temporary directory, and thus not be
    // removed in Desktop::RemoveTemporaryDirectory() if soffice process gets closed before the
    // mailer finishes using them.

    maAttachmentFiles.emplace_back(std::make_unique<utl::TempFile>(&GetBaseTempDirURL()));
    maAttachmentFiles.back()->EnableKillingFile();
    INetURLObject aFilePathObj(maAttachmentFiles.back()->GetURL());
    OUString sNewAttachmentURL = aFilePathObj.GetMainURL(INetURLObject::DecodeMechanism::NONE);
    if (osl::File::copy(sOrigAttachURL, sNewAttachmentURL) == osl::FileBase::RC::E_None)
    {
        INetURLObject url(sOrigAttachURL, INetURLObject::EncodeMechanism::WasEncoded);
        sUserVisibleName = url.getName(INetURLObject::LAST_SEGMENT, true,
            INetURLObject::DecodeMechanism::WithCharset);
    }
    else
    {
        // Failed to copy original; the best effort is to use original file. It is possible that
        // the file gets deleted before used in spawned process; but let's hope... the worst thing
        // is the absent attachment file anyway.
        sNewAttachmentURL = sOrigAttachURL;
        maAttachmentFiles.pop_back();
    }
    return sNewAttachmentURL;
}
Comment 1 Oliver Brinzing 2019-07-07 08:20:03 UTC
Created attachment 152613 [details]
calling senddoc.exe
Comment 2 Oliver Brinzing 2019-07-07 08:21:07 UTC
Created attachment 152614 [details]
senddoc.exe finished
Comment 3 Xisco Faulí 2019-07-31 11:00:52 UTC
@Jan-marek, since you fixed bug 126597, I thought you might be interested in this issue...
Comment 4 Jan-Marek Glogowski 2019-07-31 13:16:04 UTC
(In reply to Xisco Faulí from comment #3)
> @Jan-marek, since you fixed bug 126597, I thought you might be interested in
> this issue...

Maybe this is actually the same bug. The MAPI_DIALOG_MODELESS handling causes many problems. Can you or the reporter recheck this bug with a fixed LO version? Backports for 6.3 and 6.2 are pending in Gerrit, FWIW.
Comment 5 Xisco Faulí 2019-07-31 13:20:17 UTC
Dear Oliver,
Could you please try to reproduce it with a master build from http://dev-builds.libreoffice.org/daily/master/ ?
You can install it alongside the standard version.
I have set the bug's status to 'NEEDINFO'. Please change it back to 'UNCONFIRMED' if the bug is still present in the master build
Comment 6 Oliver Brinzing 2019-08-01 17:10:48 UTC
the issue still exits in:

Version: 6.4.0.0.alpha0+ (x64)
Build ID: 81963b5c68b492f6a75dd17fb0bec80e5dad9955
CPU threads: 12; OS: Windows 10.0; UI render: GL; VCL: win;
Locale: de-DE (de_DE); UI-Language: en-US
Calc: threaded
after closing
Comment 7 Oliver Brinzing 2019-08-01 17:11:22 UTC
Created attachment 153095 [details]
deleted attachment
Comment 8 QA Administrators 2021-08-01 03:54:14 UTC Comment hidden (obsolete)
Comment 9 Oliver Brinzing 2021-08-01 13:57:39 UTC
reproducible with:

Version: 7.1.5.2 (x64) / LibreOffice Community
Build ID: 85f04e9f809797b8199d13c421bd8a2b025d52b5
CPU threads: 4; OS: Windows 10.0 Build 19043; UI render: Skia/Raster; VCL: win
Locale: de-DE (de_DE); UI: de-DE
Calc: threaded
Comment 10 Mike Kaganski 2022-06-27 07:08:42 UTC
(In reply to Xisco Faulí from comment #3)
> @Jan-marek, since you fixed bug 126597, I thought you might be interested in
> this issue...

I suppose that that could be "Mike, since you implemented the CopyAttachmentcode, you could take a look here" ;)
Comment 11 Commit Notification 2022-06-27 12:00:46 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/738e0bd68c4b4286b85d3eb5c1a638247fcc26bc

tdf#126263: make sure to convert system path to file URLs

It will be available in 7.5.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2022-06-27 12:00:54 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/2a719774948fc1762e60cf7dd2ee3393c8922bb2

tdf#126263: do not try to delete non-temporary files

It will be available in 7.5.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 Commit Notification 2022-06-27 15:53:33 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "libreoffice-7-4":

https://git.libreoffice.org/core/commit/6038875c512480679c1d0ae2cae205df17bb3236

tdf#126263: make sure to convert system path to file URLs

It will be available in 7.4.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2022-06-27 15:53:42 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "libreoffice-7-4":

https://git.libreoffice.org/core/commit/530ebcde212dcedfa3435da088fcd33ff120a650

tdf#126263: do not try to delete non-temporary files

It will be available in 7.4.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Commit Notification 2022-06-29 08:08:57 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/9cef4bd3795d9f98a00baaf06005a7b3ac064517

tdf#126263: make sure to convert system path to file URLs

It will be available in 7.3.5.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 16 Commit Notification 2022-06-29 08:09:07 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/e8a2be942f1c0f9cfb008d4ab81a57a3b3fb8da0

tdf#126263: do not try to delete non-temporary files

It will be available in 7.3.5.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Gabor Kelemen (allotropia) 2022-06-29 12:30:35 UTC
A side note: while testing this with Thunderbird installed and set as default mail client, the b.pdf is named in the new mail attachment list "moz_mapi-1". The others are fine.
This is in a few days old master, before the patches, and an own build with the patches seems to still have this behavior.

This seems to have started sometime during 6.1, in 6.0 and before the attachment is named correctly b.pdf (also not removed from the original location). Maybe worth a followup report.

Based on the "senddoc.exe finished" image, this does not affect Outlook, only TB.
Comment 18 Mike Kaganski 2022-06-29 12:48:27 UTC
(In reply to Gabor Kelemen (allotropia) from comment #17)

Code pointer: use sCorrectedOrigAttachURL instead of sOrigAttachURL to obtain sUserVisibleName in CSmplMailClient::CopyAttachment [1].

[1] https://opengrok.libreoffice.org/xref/core/shell/source/win32/simplemail/smplmailclient.cxx?r=2a719774#204
Comment 19 Vasily Melenchuk (CIB) 2022-07-26 18:04:51 UTC
(In reply to Mike Kaganski from comment #18)
> Code pointer: use sCorrectedOrigAttachURL instead of sOrigAttachURL to
> obtain sUserVisibleName in CSmplMailClient::CopyAttachment [1].
> 
> [1]
> https://opengrok.libreoffice.org/xref/core/shell/source/win32/simplemail/
> smplmailclient.cxx?r=2a719774#204

It became quite important issue in case of Outlook: without --attach-name Outlook treats attachment from temporary ".tmp" file as "potentially unsafe attachment" and does not attach it.

Fix is here https://gerrit.libreoffice.org/c/core/+/137480

Mike, thx for hint!