1482728 - GetLeafName can fail (produce empty name) - ensure that doesn't cause problems

Status: NEW

(MailNews Core :: Backend, enhancement)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1482248 +++

Bug 1482248 comment 12:

The empty string comes from the failed GetLeafName call. Although NS_CopyNativeToUnicode will never fail, it means it just swallow any errors on failure even though the result is trucated. So the fundamental fix would be either

1. Fix NS_CopyNativeToUnicode so that it returns failures on error.
2. Check the return value from GetLeafName.

or

Use GetNativeLeafName on Unix-like systems.

---

Bug 1482248 comment 15:

Checking the status of GetLeafName() or using GetNativeLeafName() on Linux doesn't appear to be hard.

---

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1482728-empty-file-names.patch

Something like this? Why don't we always use GetNativeLeafName()?

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 8999516 [details] [diff] [review]
1482728-empty-file-names.patch

(In reply to Jorg K (GMT+2) from comment #1)
> Created attachment 8999516 [details] [diff] [review]
> 1482728-empty-file-names.patch
> 
> Something like this?

If the file name contains a byte sequence that is invalid as UTF-8, CopyUTF8toUTF16 will (silently) fail and the result will be truncated. So it will not fix the issue. Consider using mozilla::PathString to represent leafName. Or check the leafName using IsUTF8() before the conversion.

> Why don't we always use GetNativeLeafName()?

Because GetNativeLeafName() is lossy on Windows. So always using GetNativeLeafName() will cause problems on WIndows conversely.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> Consider using mozilla::PathString to represent leafName.
Hmm, but GetNativeLeafName() returns an nsCString. The only function I can see returning it is NativePath(). So how do we get to the leaf? And we should change nsShouldIgnoreFile() to use mozilla::PathString?

Maybe this isn't as simple to fix as I thought ;-(

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+2) from comment #3)
> (In reply to Masatoshi Kimura [:emk] from comment #2)
> > Consider using mozilla::PathString to represent leafName.
> Hmm, but GetNativeLeafName() returns an nsCString. The only function I can
> see returning it is NativePath(). So how do we get to the leaf?

mozilla::PathString is typedef'ed as nsString on Windows and as nsCString on other platforms.

> And we should change nsShouldIgnoreFile() to use mozilla::PathString?

Yes.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1482728-empty-file-names.patch (v2)

This this should be more like it. Sadly, in nsMsgBrkMBoxStore::AddSubFolders() we still need the leafName (which is currently not populated).

Note that there is a static bool nsShouldIgnoreFile(nsString& name) in nsImapMailFolder.cpp which may need some similar treatment.

Comment 6

[Jorg K (CEST = GMT+2)]

What does GetNativeLeafName() actually return? A single-byte character string (nsCString), but which encoding if not UTF-8? Locale dependent? It wouldn't be windows-1252/ISO-8859-1 on "European" systems and Shift_JIS/Windows-932 on "Japanese"?

The reporter in bug 1482248 comment #0 if French and mentions 'Ma'$'\356''tres' / "Maîtres", that would be windows-1252, so octal 356 or 0xEE.

So how do we convert this to UTF-16 to pass into AddSubFolders() for example?

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+2) from comment #6)
> What does GetNativeLeafName() actually return? A single-byte character
> string (nsCString), but which encoding if not UTF-8? Locale dependent? It
> wouldn't be windows-1252/ISO-8859-1 on "European" systems and
> Shift_JIS/Windows-932 on "Japanese"?

Linux filesystems have no concept of character encodings. Any byte sequences are valid unless they contain '\0'. So it may not be possible to convert the sequence to UTF-16.

Another option is to declare "Thunderbird supports file paths that are valid as UTF-8 regardless of the underlying file systems" and add IsUTF8() check.

Comment 8

[Jorg K (CEST = GMT+2)]

Strange, how do they display those paths then in the UI? Like 'ls' or in some file manager. Somehow that must know that 0xEE should display as î. And what if that user enters a CJK character into a file name?

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+2) from comment #8)
> Strange, how do they display those paths then in the UI? Like 'ls' or in
> some file manager. Somehow that must know that 0xEE should display as î. And
> what if that user enters a CJK character into a file name?

User-mode apps may interpret byte sequences in some manner, but it does not have to do with the kernel. Some apps use an encoding based on environment variables, so the interpretation may even vary dynamically.

Since we already hard-coding UTF-8 in some parts of our code base, adding IsUTF8() check would be the way to go IMO.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8999557 [details] [diff] [review]
1482728-empty-file-names.patch (v2)

Thanks, Masatoshi-san, I'll update the patch soon. Not our most urgent issue ;-)

Comment 11

[Jorg K (CEST = GMT+2)]

Masatoshi-san, the longer I look at this, the more confused I get. In bug 1482248 comment #9 you wrote:

On Unix, GetLeafName will convert the file path using NS_CopyNativeToUnicode.[2][3]

[2] https://dxr.mozilla.org/comm-central/rev/fe17acc6e291e54463db3ea82697c714ae5a4b27/mozilla/xpcom/io/nsLocalFileUnix.cpp#2103
[3] https://dxr.mozilla.org/comm-central/rev/fe17acc6e291e54463db3ea82697c714ae5a4b27/mozilla/xpcom/io/nsLocalFileUnix.cpp#2072

And one comment later: NS_CopyNativeToUnicode will never fail.

So what sense does it make to use GetNativeLeafName() and call IsUTF8() on the result? We might as well just use GetLeafName() and check the name returned.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1482728-empty-file-names-take2.patch

I removed
  directoryEnumerator->HasMoreElements(&hasMore);
in the first hunk. That seemed to be out of context and some copy/paste error from the past that no one ever noticed.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+2) from comment #11)
> So what sense does it make to use GetNativeLeafName() and call IsUTF8() on
> the result? We might as well just use GetLeafName() and check the name
> returned.

I meant to say call IsUTF8() on the parameter, not the result.

(In reply to Jorg K (GMT+2) from comment #12)
> Created attachment 9001964 [details] [diff] [review]
> 1482728-empty-file-names-take2.patch

Is this change necessary at all, given that the current GetLeafName() implementation will never detect failures? This change effectively just moves the IsEmpty() check from callee to caller.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #13)
> (In reply to Jorg K (GMT+2) from comment #11)
> > So what sense does it make to use GetNativeLeafName() and call IsUTF8() on
> > the result? We might as well just use GetLeafName() and check the name
> > returned.
> I meant to say call IsUTF8() on the parameter, not the result.
Hmm, which parameter? - GetNativeLeafName() doesn't have one. Perhaps you could just give me a code snippet.

> (In reply to Jorg K (GMT+2) from comment #12)
> Is this change necessary at all, given that the current GetLeafName()
> implementation will never detect failures? This change effectively just
> moves the IsEmpty() check from callee to caller.
Yes, well, the fix in nsShouldIgnoreFile() in bug 1482248 was perceived as "papering over a problem", so here I tried to find a somewhat better solution.

Maybe just having
  if (leafName.IsEmpty())
    continue;
would be enough.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1482728-empty-file-names-take2.patch (v2)

This patch has been hanging around since August 2008. I've rebased it now and reformatted to Google style.

Can you look whether this is worth taking, otherwise we close this as WONTFIX.

nsShouldIgnoreFile() will return true for an empty file name anyway, so even without the patch, continue is eventually the result.

In one case I noticed an needed call to directoryEnumerator->HasMoreElements(&hasMore);, or am I missing something? Can you please check this in the context of the file.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1482728-empty-file-names-take2.patch (v2b)

Now with a warning as requested via IRC.

Comment 17

[:aceman]

Comment on attachment 9061885 [details] [diff] [review]
1482728-empty-file-names-take2.patch (v2b)

Review of attachment 9061885 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9061885 [details] [diff] [review]
1482728-empty-file-names-take2.patch (v2b)

I'm not really happy with this. This patch doesn't fix anything. We set out to check `GetLeafName()` call sites triggered by bug 1482248 where a subsequent calls to `nsShouldIgnoreFile()` crashed. That crash has been fixed, but we still have no good concept to deal with `GetLeafName()` returning an empty file name due to encoding problems.