384803 - nsStringBundle::FormatStringFromID should return nsMemory allocated strings

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Prasad Sunkari [:prasad]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070508 Iceweasel/2.0.0.4 (Debian-2.0.0.4-1)
Build Identifier: 

The FormatString function in nsStringBundle uses nsTextFormatter::smprintf, but
nsTextFormatter does not use the shared nsMemory allocator. Instead it is recommended to free the memory it allocates using nsTextFormatter::smprintf_free.

FormatString through FormatStringFromID is being used in a lot of other modules that do not have access to nsTextFormatter (non MOZILLA_INTERNAL_API code). Lets instead use nsMemory based allocation for the result that we give out and free the string returned by smprintf ourselves!


Reproducible: Always

Comment 1

[Prasad Sunkari [:prasad]]

Attachment: FormatString uses nsCRT::strdup to allocate the result buffer.

Stores the result from nsTextFormatter::smprintf to a temporary string, nsCRT::strdup it and then free the temporary string using nsTextFormatter::smprintf_free.

Comment 2

[timeless]

Comment on attachment 268697 [details] [diff] [review]
FormatString uses nsCRT::strdup to allocate the result buffer.

>Index: intl/strres/src/nsStringBundle.cpp
>@@ -380,28 +380,37 @@ nsStringBundle::FormatString(const PRUni
>-  *aResult = 
>+  PRUnichar *text = 
>     nsTextFormatter::smprintf(aFormatStr,

I believe this can fail, so you should add:

if (!text) {
    *aResult = nsnull;
    return NS_ERROR_OUT_OF_MEMORY;
}

>+  // Instead it is recommended to free the memory it allocates using

It is more of a requirement / demand than a recommendation.

>+  // nsTextFormatter::smprintf_free.  Lets instead use nsMemory based

I think this should be "Let's"

>+  *aResult = nsCRT::strdup(text);
use NS_strdup

Comment 3

[Prasad Sunkari [:prasad]]

Attachment: FormatString uses NS_strdup to allocate the result buffer

The patch is updated as per the comments by timeless.

Comment 4

[timeless]

Comment on attachment 269068 [details] [diff] [review]
FormatString uses NS_strdup to allocate the result buffer

there should be a space between *aResult and ?

:)

Comment 5

[Prasad Sunkari [:prasad]]

timeless, can you checkin this patch for me?  I guess you could add that space when checking it in.

Comment 6

[timeless]

Comment on attachment 269068 [details] [diff] [review]
FormatString uses NS_strdup to allocate the result buffer

mozilla/intl/strres/src/nsStringBundle.cpp 	1.152