594239 - All LPTSTR arguments of Simple MAPI functions should be LPSTR

Status: RESOLVED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

According to MSDN document [1], all string arguments are LPSTR not LPTSTR.
It means charset is ANSI not UNICODE, so if Thunderbird is built with D_UNICODE, simple MAPI functions do not work at all.

[1] http://msdn.microsoft.com/en-us/library/dd296728%28v=VS.85%29.aspx

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The patch depends on the fix for bug 594224.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The patch fix bug 565041 too.

Comment 4

[David :Bienvenu]

this patch doesn't apply, either on top of the patch for bug 594224, or w/o that patch. This is the reject part:

--- msgMapiHook.cpp
+++ msgMapiHook.cpp
@@ -531,16 +449,13 @@
             if (aFiles[i].lpszFileName)
             {
               nsAutoString wholeFileName;
-                if (aIsUnicode)
-                    wholeFileName.Assign(aFiles[i].lpszFileName);
-                else
-                    ConvertToUnicode(nsMsgI18NFileSystemCharset(), (char *) aFiles[i].lpszFileName, wholeFileName);
-                // need to find the last '\' and find the leafname from that.
-                PRInt32 lastSlash = wholeFileName.RFindChar(PRUnichar('\\'));
-                if (lastSlash != kNotFound)
-                  wholeFileName.Right(leafName, wholeFileName.Length() - lastSlash - 1);
-                else
-                  leafName.Assign(wholeFileName);
+              ConvertToUnicode(nsMsgI18NFileSystemCharset(), (char *) aFiles[i].lpszFileName, wholeFileName);
+              // need to find the last '\' and find the leafname from that.
+              PRInt32 lastSlash = wholeFileName.RFindChar(PRUnichar('\\'));
+              if (lastSlash != kNotFound)
+                wholeFileName.Right(leafName, wholeFileName.Length() - lastSlash - 1);
+              else
+                leafName.Assign(wholeFileName);

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

I am sorry, my local repository was not tip of comm-central.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

This issue is not a specific issue on Windows XP. I confirmed this issue also happens on Windows 7 and Vista.

Comment 10

[David :Bienvenu]

I don't think that documentation reflects the usage of Simple MAPI, at least at the time our MAPI support was done. My understanding is that if MAPI_UNICODE was specified as a flag, then the strings were unicode, not ansi. I don't know for sure that there are MAPI clients that set the MAPI_UNICODE flag, but I suspect there are, and this patch would break them.

Is there an other way to fix the issue you see, perhaps with #ifdef _UNICODE?

Comment 11

[David :Bienvenu]

Comment on attachment 473834 [details] [diff] [review]
Revised patch

clearing review request based on prev comments.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #10)
> I don't think that documentation reflects the usage of Simple MAPI, at least at
> the time our MAPI support was done. My understanding is that if MAPI_UNICODE
> was specified as a flag, then the strings were unicode, not ansi. I don't know
> for sure that there are MAPI clients that set the MAPI_UNICODE flag, but I
> suspect there are, and this patch would break them.

Then, the MAPI clients should be fixed. Passing wchar_t* in LPSTR is insane for me.

MSDN document <http://msdn.microsoft.com/en-us/library/cc815901%28office.12%29.aspx> says:

 The current version of MAPI supports Unicode in the following methods:

 IAddrBook::Address
 IAddrBook::CreateOneOff
 IAddrBook::Details
 IAddrBook::ResolveName
 IMAPIProp::GetLastError (IAddrBook implementation only)

All simple MAPI functions do not support MAPI_UNICODE flag.

Comment 13

[David :Bienvenu]

We can't cause the MAPI clients to be fixed. We can just get complaints from users who no longer can user the app with Thunderbird.

Comment 14

[David :Bienvenu]

Isn't it up to the implementer of the simple MAPI function as to whether MAPI_UNICODE is supported? MAPI is just an interface, implemented by MAPI providers.

Can you explain your comment about building TB with D_UNICODE? We don't currently build with that defined, as I understand it. Are you proposing that we do so? I'm trying to understand what the benefit to Thunderbird as it is today of your patch.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #14)
> Isn't it up to the implementer of the simple MAPI function as to whether
> MAPI_UNICODE is supported? MAPI is just an interface, implemented by MAPI
> providers.

No. All simple MAPI functions and structures are declared with LPSTR not LPTSTR in MAPI.h. So if the user of simple MAPI (I mean 7-zip, MS Office, etc.) uses simple MAPI function with LPTSTR, compiler will output errors or warnings. At least, MSVC outputs some errors like this:

error C2664: 'MAPISENDDOCUMENTS' : cannot convert parameter 3 from (const wchar_t [12]' to 'LPSTR' to are unrelated; ...

The developer of those applications must notice their mistake.

> Can you explain your comment about building TB with D_UNICODE? We don't
> currently build with that defined, as I understand it. Are you proposing that
> we do so? I'm trying to understand what the benefit to Thunderbird as it is
> today of your patch.

I am sorry, I supposed recent TB is built with D_UNICODE. So my comment: 

> so if Thunderbird is built with
> D_UNICODE, simple MAPI functions do not work at all.

was wrong. 

All application, which is uses MAPISendDocuments() function with LPSTR, do not work with current Thunderbird whether defining D_UNICODE or not.

Comment 16

[David :Bienvenu]

(In reply to comment #15)
> 
> All application, which is uses MAPISendDocuments() function with LPSTR, do not
> work with current Thunderbird whether defining D_UNICODE or not.

Are you saying MAPI is broken in trunk builds? Because it works for me here to send a file from Word via Thunderbird trunk builds.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #16)
> (In reply to comment #15)
> > 
> > All application, which is uses MAPISendDocuments() function with LPSTR, do not
> > work with current Thunderbird whether defining D_UNICODE or not.
> 
> Are you saying MAPI is broken in trunk builds? Because it works for me here to
> send a file from Word via Thunderbird trunk builds.

Fortunately, Word uses MAPISendMail not MAPISendDocuments. In case of MAPISendMail Thunderbird converts filename from ANSI to UTF16 as you know. (But the conversion is actually not needed since MAPI functions are invoked with ANSI charset filename, path, etc. not UTF-16).

Comment 18

[Wayne Mery (:wsmwk)]

do we know anyone currently interested in mapi?

Comment 19

[:aceman]

I don't know if MAPI even works with recent Windows. Somehow I think jcranmer could know something about this stuff.

Comment 20

[Wayne Mery (:wsmwk)]

Actually, Matt in Bug 1170331 commented that MAPI does not exist as of Windows 8

Comment 21

[Jorg K (CEST = GMT+2)]

I'll take it, but I'm trying to convince Kai to rip it all out in bug 393302 since he's already switching to LPSTR there.

Comment 22

[Mike Kaganski]

(In reply to Wayne Mery (:wsmwk) from comment #20)

Actually, Matt in Bug 1170331 commented that MAPI does not exist as of
Windows 8

Just want to mention that that is severe misconception (to avoid drawing conclusions from that).

Windows has not dropped MAPI in all its existing versions, including Windows 10. Its support libraries (both 32-bit, and 64-bit, unlike e.g. TWAIN which only has 32-bit DLLs on Wondows) are there, and any compliant application may benefit from that.

What Matt had incorrectly interpreted was absence of MAPI support client libraries shipped with Exchange 2007, so that when Exchange client was installed, applications couldn't use MAPI to communicate with it.

Comment 23

[Matt]

Big oops on my part then. Sorry folks.

Comment 24

[Matt]

In my opinion the distinction which has been lost is that simple mapi has been deprecated. MAPI has not.

First we had MAPI. Then it was rewritten and the original renamed Simple MAPI and the New became Extended MAPI. Then the Simple MAPI was deprecated and the whole thing because MAPI again. But really they are two different things with the same name.

But it is also my understanding that Thunderbird was only ever written to support 12 original functions of simple MAPI. https://docs.microsoft.com/en-us/previous-versions/windows/desktop/windowsmapi/simple-mapi-functions If I am wrong, sorry for the noise. But I think part of the issue is we are implementing the 16 bit functions that were originally written for simple mapi cirica 1995, not those described now for MAPI.

Comment 25

[Mike Kaganski]

Attachment: NoTSTR.diff

A patch for current trunk - but:

1. It needs to consider changes that are likely to be done in bug 1048658 and maybe bug 1521007;
2. It uses a naive approach of converting all 8-bit strings passed to CMapiImp and nsMapiHook to UTF-16 to not change anything deeper; attachment 473834 [details] [diff] [review] uses more advanced techniques that I cannot use because of lack of deeper knowledge.

Comment 26

[Mike Kaganski]

Attachment: NoTSTR1.diff

As requested in bug 1521007 comment 8. I couldn't change indentation in changed lines, though.

Comment 27

[Mike Kaganski]

Attachment: NoTSTR2.diff

Rebased; no spaces between function names and arglists

Comment 28

[Jorg K (CEST = GMT+2)]

OK, so let's do a file-by-file comparison with Hiro's last version.

MapiDll.cpp: Removal of (faulty?) MAPI_UNICODE handling and switching of interfaces to LPSTR (same).
msgMapi.idl: Switching of interfaces to LPSTR. Hiro also removed LOGIN_PW_TYPE and added max_is(256). Shall we give that a go? That ripples through into some other files. We can also switch VerifyUserName() to LPSTR and adjust its only caller which receives that type anyway.
msgMapiHook.cpp: Like in Hiro's patch, switch to nsAutoCString strDelimChars; and lose the conversion to UTF-16. A few more things need switching too.
msgMapiMain.cpp and .h: There is no need to pass a wide string to RegisterSession(), so please apply Hiro's changes as well.

Thanks for picking this up!

Comment 29

[Joshua Cranmer [:jcranmer]]

(In reply to Matt from comment #24)

In my opinion the distinction which has been lost is that simple mapi has been deprecated. MAPI has not.

First we had MAPI. Then it was rewritten and the original renamed Simple MAPI and the New became Extended MAPI. Then the Simple MAPI was deprecated and the whole thing because MAPI again. But really they are two different things with the same name.

It's complicated, and I don't follow Windows development closely. Simple MAPI is a set of C functions that allows for very basic manipulation of a mail client. Extended MAPI is a set of IDL/COM functions that is essentially the public interface for Outlook, and is considered part of the Office SDK and not part of Windows. All of the Simple MAPI functions, save for SendMail (or, more specifically, the Unicode variant of that method), have been deprecated.

Essentially, the implication is clear: the only use of MAPI should be to send a message, or to pop up a dialog to compose a message before sending. The other functions (which support reading mail) should directly use Outlook's APIs if necessary.

Implementing Extended MAPI is a lot of work for very little gain, since it requires building an adapter between Thunderbird's representation of messages, databases, and folders and Outlook's representation.

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to Joshua Cranmer [:jcranmer] from comment #29)

Essentially, the implication is clear: the only use of MAPI should be to send a message, or to pop up a dialog to compose a message before sending. The other functions (which support reading mail) should directly use Outlook's APIs if necessary.

Joshua, can you please comment in bug 1521380.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: NoTSTR2.diff - rebased Hiro's patch

Mike, thanks for making a start. I rebased all of Hiro's patch as per comment #28.

Comment 32

[Jorg K (CEST = GMT+2)]

I found a couple more white-space sins, but I won't attach a new patch.

I've looked in to the significance of the MAPI_UNICODE flag. Apparently this is something Outlook internal, so we did the right thing to remove it here. See: https://docs.microsoft.com/en-us/office/client-developer/outlook/mapi/imapiprop-getprops

Mike can you confirm?

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ba263124fbff
Fix arguments of Simple MAPI functions that should be LPSTR. r=jorgk

Comment 34

[Jorg K (CEST = GMT+2)]

I always get second thoughts after landing something :-(

Mike, before the code used MultiByteToWideChar(CP_ACP, 0, pUserName, -1, ProfileName, .... That converted a string in a local code page to unicode, same for the password. In other words, now we're assuming that username and password are ASCII.

Digging through nsMapiHook::VerifyUserName(), it compares the username with the local part of the e-mail address and the latter is most likely ASCII. Also
https://hg.mozilla.org/comm-central/rev/ba263124fbff#l3.49
assumed that.

The password is completely ignored, so there is doesn't matter.

So I guess we are OK. We might consider converting an ANSI string into UTF-8, but we can leave this for bug 1521380 where we'll see whether login works at all.

Comment 35

[Mike Kaganski]

(In reply to Jorg K (GMT+1) from comment #32)

I've looked in to the significance of the MAPI_UNICODE flag. Apparently this is something Outlook internal, so we did the right thing to remove it here. See: https://docs.microsoft.com/en-us/office/client-developer/outlook/mapi/imapiprop-getprops

Mike can you confirm?

Yes, I am sure your analysis is right.
Given that MS does have an (initially undocumented) hack how to use Unicode in MAPISendMail (using CP_UTF8 in MapiMessage), and that (part of) MS Office (Word) actually uses it sending documents (!), I don't believe that they ever had that hybrid approach with MAPI_UNICODE TB used to implement. It was either a wishful thinking on TB part, or support for some home-grown hack for some third-party application... well - if the latter, then that application should have moved to the W function anyway.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0befd65ff82b
Follow-up: Don't pass null to nsDependentCString. r=me DONTBUILD

Comment 37

[Mike Kaganski]

(In reply to Jorg K (GMT+1) from comment #34)

Digging through nsMapiHook::VerifyUserName(), it compares the username with the local part of the e-mail address and the latter is most likely ASCII. Also
https://hg.mozilla.org/comm-central/rev/ba263124fbff#l3.49
assumed that.

The password is completely ignored, so there is doesn't matter.

So I guess we are OK. We might consider converting an ANSI string into UTF-8, but we can leave this for bug 1521380 where we'll see whether login works at all.

I agree. I also relied on that ASCII conversion when made my tentative patch :-)

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: NoTSTR2.diff - ESR60 version

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: NoTSTR2.diff - ESR60 version

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: NoTSTR2.diff

One more time, now folded the second patch.

Comment 41

[Jorg K (CEST = GMT+2)]

TB 65 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/c6a688894f61e63f9556cad0870372d469611d60
https://hg.mozilla.org/releases/comm-beta/rev/f978492d2a2e85bec7383b55228dd67ac22eca9a

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a895eda2b257
Follow-up: remove trailing space. rs=white-space-only DONTBUILD

Comment 43

[Jorg K (CEST = GMT+2)]

TB 60.5.2 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/eb1144931b6a67a8e7be00876737acf5140f9bf6