Closed Bug 1410973 Opened 3 years ago Closed 3 years ago

nsMsgI18NFileSystemCharset() should not return const char*

Categories

(MailNews Core :: Internationalization, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: hsivonen, Assigned: jorgk-bmo)

References

Details

(Keywords: sec-audit)

Attachments

(3 files, 1 obsolete file)

nsMsgI18NFileSystemCharset() returns a C string whose storage belongs to a  static nsAutoCString. That's a use-after-free waiting to happen.

The method should be deleted as obsolete, return const mozilla::Encoding* or have an nsACString& outparam.
How about an nsCString/nsAutoCString return value? We have many callers like this:
  nsresult rv = ConvertToUnicode(nsMsgI18NFileSystemCharset(), ...
and could save declaring extra variables. Too unconventional?
Keywords: sec-audit
Henri, perhaps you've missed my comment #1.
Flags: needinfo?(hsivonen)
I don't know our string classes well enough to say if returning them would be safe. For sure, they aren't designed to be used that way and are instead designed to be used as outparams. Therefore, declaring the extra variable and using the outparam convention seems like the safe way to go.
Flags: needinfo?(hsivonen)
I have a function
  nsAutoCString MsgExtractQueryPart(nsAutoCString spec, const char* queryToExtract)
and no one has complained so far.

There are other functions in the system that return nsAutoCString, for example
http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/widget/gtk/nsFilePicker.cpp#143
http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/browser/components/about/AboutRedirector.cpp#108

Eric, any objections to returning an nsAutoCString from nsMsgI18NFileSystemCharset()?
Flags: needinfo?(erahm)
(In reply to Henri Sivonen (:hsivonen) from comment #0)
> nsMsgI18NFileSystemCharset() returns a C string whose storage belongs to a 
> static nsAutoCString. That's a use-after-free waiting to happen.
> 
> The method should be deleted as obsolete, return const mozilla::Encoding* or
> have an nsACString& outparam.

Is the concern that we're going to use it in another static destructor? That seems pretty unlikely...

(In reply to Henri Sivonen (:hsivonen) from comment #3)
> I don't know our string classes well enough to say if returning them would
> be safe. For sure, they aren't designed to be used that way and are instead
> designed to be used as outparams. Therefore, declaring the extra variable
> and using the outparam convention seems like the safe way to go.

It's safe to return a string, the main concern is that we might do an extra copy. We recently added a move constructor and I generally expect all supported compilers will do the right thing and avoid the extra copy *with the exception of auto strings.* Really at this point it's just a matter of style.

(In reply to Jorg K (GMT+2) from comment #4)
> I have a function
>   nsAutoCString MsgExtractQueryPart(nsAutoCString spec, const char*
> queryToExtract)
> and no one has complained so far.
> 
> There are other functions in the system that return nsAutoCString, for
> example
> http://searchfox.org/mozilla-central/rev/
> dd47bee6468de7e1221b4d006342ad6b9813d0e5/widget/gtk/nsFilePicker.cpp#143
> http://searchfox.org/mozilla-central/rev/
> dd47bee6468de7e1221b4d006342ad6b9813d0e5/browser/components/about/
> AboutRedirector.cpp#108
> 
> Eric, any objections to returning an nsAutoCString from
> nsMsgI18NFileSystemCharset()?

Returning an `nsCString` would be fine, don't use an auto string (it'll end up being less efficient). Regardless of whether you use an outparam or not you should change the type of `fileSystemCharset` to `nsCString` so as to avoid allocing/copying on every call (internally we have to make a copy on assignment if we're using the inline buffer).

After looking over the usage of `nsMsgI18NFileSystemCharset` [1] I think all of this is a bit academic. Just have it return a `const nsACString&`. If something wants to store it we'll do a safe copy, but as-is it's pretty much just used as a temp var.

[1] http://searchfox.org/comm-central/search?q=symbol:_Z26nsMsgI18NFileSystemCharsetv&redirect=false
Flags: needinfo?(erahm)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #5)
> (In reply to Henri Sivonen (:hsivonen) from comment #0)
> > nsMsgI18NFileSystemCharset() returns a C string whose storage belongs to a 
> > static nsAutoCString. That's a use-after-free waiting to happen.
> > 
> > The method should be deleted as obsolete, return const mozilla::Encoding* or
> > have an nsACString& outparam.
> 
> Is the concern that we're going to use it in another static destructor? That
> seems pretty unlikely...

The concern is that there's nothing that guarantees that something doesn't hold onto the char* after the contents of the static nsAutoCString change. Maybe in this case the contents don't change and nothing holds onto the pointer for too long, so stuff doesn't break. Still, on surface, it looks like a bad pattern.
Eric, could you help us out here. Thanks in advance.

BTW, should I use |.Assign()| or '=' as I've done here?
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8922965 - Flags: review?(erahm)
Comment on attachment 8922965 [details] [diff] [review]
1410973-nsMsgI18NFileSystemCharset.patch (v1)

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

Looks like a good start, I've added some notes on a slightly more efficient implementation. If you don't wish to pursue that please ask for a re-review as I think this technically works as-is.

::: mailnews/base/util/nsMsgI18N.cpp
@@ +124,5 @@
>    return NS_OK;
>  }
>  
>  // Charset used by the file system.
> +nsCString nsMsgI18NFileSystemCharset()

I'm sorry if I wasn't clear before.

The best option is to have this return a `const nsACString&` and then update `nsMsgI18NConvertFromUnicode` and `nsMsgI18NConvertToUnicode` [1] to take a `const nsACString& aCharset` (similar to the other params). You'll need to update other callers to wrap raw strings in an `nsDependentCString`.

You might want to add an overload to `ConvertToUnicode` that takes a `const nsACString&` as well.

For cases that are passing a constant string in (ie "UTF-8") you can use NS_LITERAL_CSTRING(...).

For instances where you want to use `get()` you'll need to use `PromiseFlatCString(...).get()` instead.

[1] http://searchfox.org/comm-central/source/mailnews/base/util/nsMsgI18N.cpp#36-88

::: mailnews/import/outlook/src/MapiMessage.cpp
@@ +723,5 @@
>          m_mimeCharset.Assign(charset);
>      }
>    }
>    if (!bFoundCharset) { // Use system default
> +    const nsCString charset = nsMsgI18NFileSystemCharset();

This would be `const nsCString&`

::: mailnews/import/text/src/nsTextAddress.cpp
@@ +41,5 @@
>  {
>    nsAutoCString charset;
>    nsresult rv = MsgDetectCharsetFromFile(aFile, charset);
>    if (NS_FAILED(rv)) {
> +    charset = nsMsgI18NFileSystemCharset();

Assignment with `operator =` is fine.
Attachment #8922965 - Flags: review?(erahm) → feedback+
From experience, any clean-up in Mailnews spirals out of control. You start off doing a small thing and it blows up into to big thing.

I'll comment on the patch in the next comment.
Attachment #8922965 - Attachment is obsolete: true
Attachment #8923094 - Flags: feedback?(erahm)
Comment on attachment 8923094 [details] [diff] [review]
1410973-nsMsgI18NFileSystemCharset.patch (v2)

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

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +1117,5 @@
>      if (charset != nullptr)
>      {
> +      nsMsgI18NConvertToUnicode(nsDependentCString(charset),
> +                                nsCString(stringToMatch),
> +                                utf16StrToMatch);

Why the nsCString() here?

::: mailnews/base/util/nsMsgI18N.cpp
@@ +37,1 @@
>                                       const nsString& inString,

Why isn't that a const nsAString&?

@@ +67,1 @@
>                                     const nsCString& inString,

Why not a const nsACString&

::: mailnews/base/util/nsMsgI18N.h
@@ -177,5 @@
> -inline void ConvertRawBytesToUTF8(const nsCString& inString,
> -                                  const char* charset, nsACString& outString)
> -{
> -    return nsMsgI18NConvertRawBytesToUTF8(inString, charset, outString);
> -}

I removed all those confusing helper functions.

::: mailnews/import/becky/src/nsBeckyUtils.cpp
@@ +295,5 @@
>  
>    nsCOMPtr<nsIConverterInputStream> convertedInput =
>      do_CreateInstance("@mozilla.org/intl/converter-input-stream;1");
> +  convertedInput->Init(source,
> +                       PromiseFlatCString(nsMsgI18NFileSystemCharset()).get(),

What's the difference between PromiseFlatCString() and nsPromiseFlatCString(). I've just used the latter in bug 1412421:
https://hg.mozilla.org/comm-central/rev/6982e252d9b9#l1.18
Should that have been PromiseFlatCString()?

::: mailnews/import/outlook/src/MapiMessage.cpp
@@ +723,5 @@
>          m_mimeCharset.Assign(charset);
>      }
>    }
>    if (!bFoundCharset) { // Use system default
> +    const nsCString charset(nsMsgI18NFileSystemCharset());

|const nsCString&| as you had suggested didn't work here.
OK, changing the interfaces to "A" strings.
Attachment #8923096 - Flags: feedback?(erahm)
Gentle review/feedback ping.
Flags: needinfo?(erahm)
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
Comment on attachment 8923094 [details] [diff] [review]
1410973-nsMsgI18NFileSystemCharset.patch (v2)

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

Looks good, sorry this ended up touching so much!

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +1117,5 @@
>      if (charset != nullptr)
>      {
> +      nsMsgI18NConvertToUnicode(nsDependentCString(charset),
> +                                nsCString(stringToMatch),
> +                                utf16StrToMatch);

stringToMatch is an nsACString, but ConvertToUnicode takes an nsCString. Generally we recommend that function signatures use nsA[C]String instead of their concrete counterparts (nsString, nsAutoString, etc).

::: mailnews/base/util/nsMsgI18N.cpp
@@ +37,1 @@
>                                       const nsString& inString,

Probably a mistake.

@@ +67,1 @@
>                                     const nsCString& inString,

Again, probably a mistake.

::: mailnews/base/util/nsMsgI18N.h
@@ -177,5 @@
> -inline void ConvertRawBytesToUTF8(const nsCString& inString,
> -                                  const char* charset, nsACString& outString)
> -{
> -    return nsMsgI18NConvertRawBytesToUTF8(inString, charset, outString);
> -}

+1

::: mailnews/import/becky/src/nsBeckyUtils.cpp
@@ +295,5 @@
>  
>    nsCOMPtr<nsIConverterInputStream> convertedInput =
>      do_CreateInstance("@mozilla.org/intl/converter-input-stream;1");
> +  convertedInput->Init(source,
> +                       PromiseFlatCString(nsMsgI18NFileSystemCharset()).get(),

Not a 100% sure (PromiseFlatCString is a function, nsPromiseFlatCString is an object). Generally we use the shorter version.

::: mailnews/import/outlook/src/MapiMessage.cpp
@@ +723,5 @@
>          m_mimeCharset.Assign(charset);
>      }
>    }
>    if (!bFoundCharset) { // Use system default
> +    const nsCString charset(nsMsgI18NFileSystemCharset());

Okay that's fine, my guess |const nsACString&| would have worked but it's not a big deal.
Attachment #8923094 - Flags: feedback?(erahm) → feedback+
Comment on attachment 8923096 [details] [diff] [review]
1410973-more.patch

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

Nice! That was an easy cleanup.
Attachment #8923096 - Flags: feedback?(erahm) → feedback+
Thanks Eric. I will address the issues, merge the patches and present them for review once my contract is extended.
Flags: needinfo?(erahm)
Looking at the comments in context in the "Splinter Review", there was nothing I needed to change, so I've just merged the two patches.

I left the 
  const nsCString charset(nsMsgI18NFileSystemCharset());
instead of the suggested
  const nsACString& charset(nsMsgI18NFileSystemCharset());
since two lines lines down there is charset.get() which would need a PromiseFlatCString() if the |const nsACString&| were used.
Attachment #8927935 - Flags: review?(erahm)
Comment on attachment 8927935 [details] [diff] [review]
1410973-nsMsgI18NFileSystemCharset.patch (v2) + more patch

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

r=me
Attachment #8927935 - Flags: review?(erahm) → review+
https://hg.mozilla.org/comm-central/rev/bc4dff04bede39d57bc4353f8eeae800fb470877

Fixed in TB 59, not TB 58.
Assignee: nobody → jorgk
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
Group: mail-core-security → core-security-release
Target Milestone: Thunderbird 58.0 → Thunderbird 59.0
Depends on: 1510028
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.