User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7 Build Identifier: Mozilla Thunderbird 1.5.0.7 Build 20060909 If you are trying to indirectly attach a file outside thunderbird(using SimpleMAPI, for example: right-clicking on a file on Windows Explorer and choosing "Send To" -> "Mail Recipient") and a stale different file using the same name already exists in the thunderbird's temporary mapi attachment folder (in my computer this is "C:\Documents and Setting\myusername\Local Settings\Temp\moz_mapi") then Thunderbird silently ignores the new attachment and instead wrongly uses the one already found it moz_mapi directory. This can cause users to send e-mail with the wrong attachment in them, if there are stale files in moz_mapi directory. Reproducible: Always

Reproduced with TB 1.5.0.7, 2b1-1014, 3a1-1011, Win2K. I'm not sure what conditions lead to the file being left in that temp directory -- every time I tested this, the temp file was no longer present after sending the message.

There is a special case where stale files are created with another bug I submitted, bug 356919

(In reply to comment #1) > I'm not sure what conditions lead to the file being left in that temp directory At least "R" attribute of sent file will produce the phenomenon on MS Win. See Bug 356919 Comment #3, please.

We worked around this bug by making moz_mapi an NTFS junction point for the parent temporary directory.

This is probably a bug in HandleAttachments in mozilla/mailnews/mapi/mapihook/src/msgMapiHook.cpp: pTempFile is created with a unique filename, but leafName is not updated with the new unique value. So the original file is copied to the default leafname (which fails silently), and then the old version of the file is attached. We need something like this at the end of the relevant if block: pTempFile->GetLeafName (leafName);

This is broken for me as well (I've got TB3beta1 and 3beta2 both installed) This seems to work with 3b1 but is broken in 3b2

(In reply to comment #6) > This is broken for me as well (I've got TB3beta1 and 3beta2 both installed) > This seems to work with 3b1 but is broken in 3b2 Sorry, in my case, 3b2 simply ignores the SimpleMAPI altogether - it isn't a case of it attaching the wrong file.

This very annoying bug is still in TB3b2.

This might be dataloss if the wrong file was inadvertently attached and/or sent.

Indeed, it was data loss for us. Customers ended up getting the wrong quote letters a few times. (Man, was that embarrassing!) Using an NTFS junction point to make the moz_mapi folder an alias for the profile temporary folder fixes the issue in the short term, but it's a nasty hack.

when is this bug going to be fixed?

We are currently experiencing this bug with our users. It is very annoying as they try to send out different versions of documents from our DMS (i.e. the same filename but different content) and end up with the same version everytime. The sending user does not see any difference when composing the email.

I'm aware mapi is microsoft-related, but does this bug only affect windows systems? Otherwise platform/OS should be changed accordingly.

This is just a patch version of comment #5, but I neither know this code nor know how to repro it. If someone can repro this bug easily with nightly builds, we could do a try-server build. (which doesn't mean we'd take the patch in the tree at this point, as our test coverage in this area is bad, I fear)

http://mxr.mozilla.org/comm-central/source/mailnews/mapi/mapihook/src/msgMapiHook.cpp#560 557 { 558 rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0777); 559 NS_ENSURE_SUCCESS(rv, rv); 560 pTempFile->Remove(PR_FALSE); // remove so we can copy over it. 561 } 562 I think the intent of this was to rename the file with name we want and then delete the original but it is only deleting the new named old file so the later CopyTo() fails because it still exists. I don't know why we want to keep temp files anyway. If someone is using this the old file then it won't rename anyway I don't think. lets just delete the old file and copy the new one in.

bug 77810 related? this may help alleviate Bug 389132 - 100% CPU for long time when forwarding multiple messages as attachment, if many garbages of nsmail-N.tmp remain in \Temp or /tmp directory

This eliminates 'send to' creating a temp file which, BTW, never gets deleted. If you create a message compose and drag and drop or add attachment any other way it uses the original file so why not this method as well. FWIW, OExp uses the original file also in its 'send to'.

My understanding was that we made a copy of the file because "Send To" is a synchronous API and we don't own the file after the function returns; OExp is able to create a compose window modally to work around the problem.

(In reply to comment #20) > My understanding was that we made a copy of the file because "Send To" is a > synchronous API and we don't own the file after the function returns; OExp is > able to create a compose window modally to work around the problem. Can we lock the file while under control of composer? If not then I can do a patch to fix the bug and keep the temp file.

(In reply to comment #21) > (In reply to comment #20) > > My understanding was that we made a copy of the file because "Send To" is a > > synchronous API and we don't own the file after the function returns; OExp is > > able to create a compose window modally to work around the problem. > Can we lock the file while under control of composer? Not as far as I know. > If not then I can do a patch to fix the bug and keep the temp file. Please do.

Comment on attachment 401576 [details] [diff] [review] Using original file for attachment At least with Outlook 2000 (best test I could come up with, sorry), if you use Explorer to Send To, then the window that's opened doesn't let you open other windows; various errors appear when you try. I assume this is so that the MAPI call doesn't return until the window is closed (either via sending, saving or cancelling).

I'll do a patch 'deleting the old file and renaming the new file'. note in meantime in case you have comments on these (not necessary): 1. nothing in temp file is deleted, ever (need new bug); 2. this mapi send to procedure will be only one that uses temp files for attachments, drag and drop and adding attachments thru menu both uses original file. (haven't tried but if we can open file on front end with limited shared access and close file when compose window closes, we will be locking file while composer in use). Maybe this would be new bug too.

tested it to work with windows. Does delete existing file with same name in temp directory.pass In case of file in use: did a python Open( ,'r+) an existing temp file with same name before 'send to' and it returned an error and the 'send to' file was copied to temp dir under unique name. pass Note, we don't do this with other methods of adding attachments to composer. Only use original files and don't lock them. Also this code does not delete files in directory when done.

(In reply to comment #25) > Created an attachment (id=404768) > Note, we don't do this with other methods of adding attachments to composer. > Only use original files and don't lock them. Hmm, so if I use MAPI to send two different files with the same name without closing the compose window in between, then things will break?

Comment on attachment 404768 [details] [diff] [review] delete existing if possible or else use new name >- pTempFile->Remove(PR_FALSE); // remove so we can copy over it. Don't we need to keep this? By my reading, CopyTo will fail it we don't remove. >+ pTempFile->GetLeafName(leafName); So, this is the actual fix, isn't it! If you prefer, I'll gladly r+ a patch with just this fix in it and we can decide what to do about deleting separately.

(In reply to comment #27) > (From update of attachment 404768 [details] [diff] [review]) > >- pTempFile->Remove(PR_FALSE); // remove so we can copy over it. Can someone explain to me what exactly is removed in this line? Is it deleting an actual existing temp file? Or are we just emptying a path variable for the file name? (If it's a file, where does it come from? If we haven't created it in the same run, this will have similar effect as bug 344034?) I think we should NOT be deleting any files that we haven't created in the same run.

I think the patch for this problem needs to meet at least following requirements, if we want to avoid trouble like in bug 344034. When user attaches file "filename.ext" via mapi: - try to create immediate snapshot copy of "filename.ext" in moz_mapi/, but - do not delete any existing temp files of that name (-> bug 344034) - do not rename any existing temp files of that name (-> bug 344034) -> make sure to use unique file name that doesn't exist yet - The unique file name should include original "filename" as substring, e.g. "filename-1.ext" or "filename.847565.ext". It should not be all cryptic like "nsmail1.ext". Watch out for bug 407577 (Thunderbird incorrectly handles Simple MAPI attachments if they have a '-' in the file name).

(In reply to comment #26) > Hmm, so if I use MAPI to send two different files with the same name without > closing the compose window in between, then things will break? yes, why I say stick with attaching original regarding other comments questions about code I modified the block dealing with existing temp files of same name. It now deletes the file, if that fails it creates new unique file and whichever filename is successful that is the one used for the copyto.

(In reply to comment #29) > - do not delete any existing temp files of that name (-> bug 344034) good luck. Our temp files are never deleted. Maybe on TB first use we can empty our directory. OEXP and IE seem to have notice problem here as they never seem to empty their directories, but just expect user to 'clean disk' with those directories in the cue so to speak.

(In reply to comment #31) > (In reply to comment #29) > > - do not delete any existing temp files of that name (-> bug 344034) > > good luck. Our temp files are never deleted. Maybe on TB first use we can > empty our directory. it is also a security issue. see Bug 58979 comment 13 store all compose temp files in directory under /tmp, and remove that directory on quit and Bug 377630 - Attachment filename disclosure in /tmp OT perhaps, but the wider cluster of these bugs (which needs some cleanup) is Bug 235432 Mailnews leaves unused nsqmail.tmp (nsqmail-*.tmp) files in temporary folder after quit Bug 389132 - 100% CPU for long time when forwarding multiple messages as attachment, if many garbages of nsmail-N.tmp remain in \Temp or /tmp directory Bug 368380 - lots of temp files left around Bug 74047 - use a subdirectory under the temp directory to hold all the compose temp files Bug 220179 - Temporary files in mail folders on hard drive are not removed if compact interruped Bug 299655 - /tmp files left behind, some world-readable Bug 392592 - Temporary files used to save mail attachments are not deleted when canceling in file picker Bug 58580 - temp files from sending drafts or posting news are create with bad permissions > OEXP and IE seem to have notice problem here as they > never seem to empty their directories, but just expect user to 'clean disk' > with those directories in the cue so to speak. indeed, but a bit optimistic to assume users will do that :)

I use sendto of same file name on daily basis and can't live with bug in TB3. This should be +block TB3. And least problematic would be send-original-file patch. Most windows users with business application of TB will be in same boat and that means dataloss at *best*, and at worse, scenarios of unintentional sending of confidential files with generic names. report.pdf or employeereview.txt. etc. Problems similar to previous comments will be popping up all over if something isn't in TB3. Sorry for bug spam but we have a -TB3blocking.

Comment on attachment 404768 [details] [diff] [review] delete existing if possible or else use new name Call me a coward, but I think we should only delete our new temporary file for now.

(In reply to comment #30) > (In reply to comment #26) > > Hmm, so if I use MAPI to send two different files with the same name without > > closing the compose window in between, then things will break? > > yes, why I say stick with attaching original Phil, even attaching the original file (i. e. "attach original later when sending") will not solve the current problem of your patch. It will still break the scenario you describe in comment #33, when user wants to send 2 versions of same file. Scenario/Steps, why "attach later when sending" doesn't work: - via mapi (e.g. from within winword or explorer) user attaches c:\test.txt with content "hello world" -> 1st compose window -> In 1st compose win, TB only has a link to c:\test.txt and does not keep a tmp copy -> in the time before sending, a lot of things can happen so that file cannot be retrieved when sending (renamed, removed, deleted etc. see list below). - user does changes to C:\test.txt, content is now "modified text" - via mapi, user sends same file again -> 2nd compose window -> In 2nd compose win, TB only has a link to c:\test.txt and does not keep a tmp copy - user now sends both mails -> both mails will have original "C:\test.txt" attached upon sending, i.e. both mails will have "modified text" So from user's point of view, two *different* versions of file have been attached, but in reality TB has two mails with the same link to the original file, and will attach only the second/last version to both mails upon sending -> dataloss, unwanted leak of information (breach of privacy) Generally speaking, attaching the original file (i. e. "attach original source file later when sending") has a lot of problems. To name but a few: - source file might have been renamed by the time you actually send - source file might have been deleted - source file might have been on removable media which is gone - source file might have changed and user might send sensitive data without knowing, because the changes weren't there when the file was attached - can't compose with two different versions of same file simultaneously Therefore, after long discussion in bug 378046, developers have requested to use "attach snapshot immediately" as default (see bug 378046, comment #38). This is what TB currently does for mapi attachments. > regarding other comments questions about code I modified the block dealing with > existing temp files of same name. It now deletes the file, if that fails it > creates new unique file and whichever filename is successful that is the one > used for the copyto. As pointed out by Neil's and my comments, deleting existing files that you haven't just temporarily created yourself in the same process will always break things. I think the desired patch just needs very few lines, and few changes compared to your current patch, as per Neil's comment #27 and comment #34.

Comment on attachment 404768 [details] [diff] [review] delete existing if possible or else use new name Phil, it would be great if you could change your current patch like this: (please let me know if I overlooked sth or suggest sth. wrong) >- // rename or copy the existing temp file with the real file > name Please re-include this line ("this needs to stay"), although I'm not sure if this is accurate description of what actually happens >+ // delete any existing temp file with the same file name >+ // if error on delete, create unique filename Please remove this change ("this needs to go.") >- rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, > 0777); this needs to stay >+ rv = pTempFile->Remove(PR_FALSE); >+ if (NS_FAILED(rv)) >+ rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, > 0777); these lines need to go >- pTempFile->Remove(PR_FALSE); // remove so we can copy > over it. this needs to stay >+ pTempFile->GetLeafName(leafName); this is the patch Could you try if this fixes the problem? I'd be curious to know because I don't have testing environment to test myself.

Comment on attachment 401481 [details] [diff] [review] tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name) (In reply to comment #36) Well, so basically that leaves us with the one-line change proposed bei Neil in comment #27. Which is basically the one-line change proposed by David Ascher in attachment 401481 [details] [diff] [review]. Has this ever been tested?

Comment on attachment 404768 [details] [diff] [review] delete existing if possible or else use new name As per comment #26 to comment #30, this patch will break when using mapi to compose two mails with different versions of same source file. It's also review- per Neil's comment #34.

Neil The first untested patch is ok to go in. We will now have to deal with bugs asking why attachments using sendto have "-1" or some number appended to their filenames and why the other methods of attaching files do not. But this patch does fix the bug so better this then nothing.

(In reply to comment #39) > Neil > The first untested patch is ok to go in. We will now have to deal with bugs > asking why attachments using sendto have "-1" or some number appended to their > filenames and why the other methods of attaching files do not. Phil, you mean that not only the temp file in moz_mapi, but also the display name of attachments in attachment panel will show the "-1"? I thought that tmp-filname and display name are two separate things, now that we can use context menu to rename attachments in Tb3?

(In reply to comment #40) > Phil, you mean that not only the temp file in moz_mapi, but also the display > name of attachments in attachment panel will show the "-1"? I thought that yes that is what happens. There would have to be another bug for handling the naming of the attachment file in the front end.

sorry but I omitted in my comment 39 that I did test that patch and it fixes this bug

Considering how the name change may disturb many people, myself included, how about creating a new directory of a random name inside the temporary moz_mapi directory, that will contain the filename unchanged? I'am not sure how this will complicate things though, any ideas?

Phil, thanks for your feedback. (In reply to comment #41) > (In reply to comment #40) > > Phil, you mean that not only the temp file in moz_mapi, but also the display > > name of attachments in attachment panel will show the "-1"? I thought that > yes that is what happens. There would have to be another bug for handling the > naming of the attachment file in the front end. Could you actually *test* if our unique tmp filename (file-1.ext) really gets promoted into the front end file name? If yes, could you post an mxr link where the display names are set, and a few notes how our temp file name gets there? What I find is that renaming the attached file in the front-end does NOT change the tmp file name in moz_mapi. So we definitely have two separate entities (tmp file name vs. display file name), perhaps you can find out how to keep them separate from the start and promote only the original file name to the front end? That would be great. (In reply to comment #43) > Considering how the name change may disturb many people, myself included, how > about creating a new directory of a random name inside the temporary moz_mapi > directory, that will contain the filename unchanged? > I'am not sure how this will complicate things though, any ideas? It would work, but surely that's not what we want. Considering that we are not yet cleaning up the files, one extra tmp subfolder for each attachment would add a lot of clutter to the mess.

Tim Retout, you seem to know this code (comment #5). Your comment fixes the main bug, but now there is a new problem since our unique tmp file name (like "attachment-1.doc") gets promoted to the front end, but what we want is just "attachment.doc". However, user can rename file from frontend, so there must be a separate entitiy for the display name. Any ideas where to look or how to fix this remaining problem?

We should have this patch blessed and landed in order to cure the more serious problem of dataloss. Another bug can be filed for the names. I think that will be in front end code anyway, not mapi code.

Agreed. Let's get the basic patch in to fix dataloss, and privacy issue which is probably worse, when wrong version with sensitive data gets sent to wrong recipient. Neil's comments from last review have been addressed. Neil, could you review this?

Comment on attachment 401481 [details] [diff] [review] tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name) Neil, could you review the first patch provided by David Ascher? The patch fixes the dataloss and privacy issue where wrong versions of files get sent; however, in these cases the filename of the attachment in the attachment pane will be changed to sth. like attachment-1.doc. If User doesn't like that, he can rename the file from attachment pane, back to attachment.doc, without breaking anything data-wise. I think we should stamp out the dataloss and privacy issue for 3.0 and accept the display name issue until 3.1.

For judging the severity of this bug, please consider that we will /always/ send the wrong file whenever user composes, via mapi, two different mails to which he attaches two different files of the same name (one each, even from different locations), or two different versions of the same file (probably, yet not confirmed, only in cases where the second mail incl. attachment is composed before first mail is sent).

Comment on attachment 401481 [details] [diff] [review] tested patch (in case where tmp file already exists, ensure that leafName gets updated with new unique tmp file name) >+ pTempFile->GetLeafName (leafName); Nit: unnecessary space before (

(Sorry for the delay but my email server is down and it's not so easy to work out from bug queries which bugs to look at.)

this should fix the name but my static build is screwed up so I'll test later and reset flags if this doesn't get checked in first.

Comment on attachment 416332 [details] [diff] [review] fixes name verified this fixes the same name problem.

Comment on attachment 416332 [details] [diff] [review] fixes name Some nits: >+ nsCOMPtr<nsIMsgAttachment> attachment = do_CreateInstance(NS_MSGATTACHMENT_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ attachment->SetName(leafName); Can you add a blank line after here please? > nsCOMPtr <nsIFile> pTempFile; > rv = pTempDir->Clone(getter_AddRefs(pTempFile)); > if (NS_FAILED(rv) || (!pTempFile) ) > return rv; > > pTempFile->Append(leafName); > pTempFile->Exists(&bExist); > if (bExist) > { > rv = pTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0777); > NS_ENSURE_SUCCESS(rv, rv); > pTempFile->Remove(PR_FALSE); // remove so we can copy over it. >+ pTempFile->GetLeafName (leafName); This still doesn't need a space before the ( > } A blank line after here would be nice. Better still, outdent these 13 lines to match the lines before and after.

forward r+=neil

checkin-needed?

r+=Neil

Comment on attachment 416489 [details] [diff] [review] fix bug plus fixed 'some' existing whitespace this function needs reformatting of the all the lines. and the file isn't much better. I'll do it if ok'd

Comment on attachment 416489 [details] [diff] [review] fix bug plus fixed 'some' existing whitespace Thx for the patch. I know this is just moved code, but can you lose the space between nsCOMPtr and <nIFile, and lose the () around !pTempFile, and the space after the close paren. + nsCOMPtr <nsIFile> pTempFile; + rv = pTempDir->Clone(getter_AddRefs(pTempFile)); + if (NS_FAILED(rv) || (!pTempFile) )

(In reply to comment #60) > (From update of attachment 416489 [details] [diff] [review]) > Thx for the patch. I know this is just moved code, but can you lose the space > between nsCOMPtr and <nIFile, and lose the () around !pTempFile, and the space > after the close paren. > > > + nsCOMPtr <nsIFile> pTempFile; > + rv = pTempDir->Clone(getter_AddRefs(pTempFile)); > + if (NS_FAILED(rv) || (!pTempFile) ) David, it didn't match the rest of the function formatting, should I fix all the format nits in this function (see previous patch), or just this one.

whitespace etc. r+=neil sr+=bienvenu

Checked in: http://hg.mozilla.org/comm-central/rev/fa4c75b79bf6

Comment on attachment 417400 [details] [diff] [review] updated per sr for checkin bug 356808 comment 33 is reasoning for maintenance fix. This is not data loss it is data misrepresentation. We would be better off breaking mapihook then letting the bug hang around. I have users emailing me asking for a way to patch.

sorry, meant to add. This patch will apply as is to 1.9.1 and nsIMsgAttachment that is referenced in this patch has not changed

Checked into 1.9.1: http://hg.mozilla.org/releases/comm-1.9.1/rev/a6b19b17aeae

This bug should have its own automated testcase (to be created). IIRC, there was considerable uncertainty about the code and the patches, and it's about dataloss / breach of privacy, so it's really worth the work of creating the test.