281692 - nsXPCOMGlue.cpp acts incorrectly on Environment Value (XPCOM_SEARCH_KEY)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Wood (:Callek)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp&rev=1.27&root=/cvsroot&mark=485,493,496#480

In this file (as my bonsai link shows)

(line 493) The effects of PR_smprintf are never stored nor de-allocated
correctly, since the pointer is never set to a variable.

Need a temp |const char *| variable to store the result at.
/*============*/
temp_env = PR_smprintf(XPCOM_SEARCH_KEY "=%s" XPCOM_ENV_PATH_SEPARATOR "%s",
                       grePath, path);
if (temp_env){
  if (PR_SetEnv(temp_env) == PR_SUCCESS){
    /* move line 485 here */
    if (spEnvString) PR_smprintf_free(spEnvString);
    spEnvString = temp_env; /* Leaks on Purpose */
  } else
    PR_smprintf_free(temp_env);
}
/*==========*/
To use code which should work in theory (since some OS's keep a static pointer
to the value sent to Set Environment rather than copy it)

And slightly later in the code;
(line 503)
if (PR_SetEnv(sEnvString) == PR_SUCCESS)
  if (spEnvString) PR_smprintf_free(spEnvString);

It might be worth storing the static (leaking) buffer all the time, since as was
said some OS's (according to nspr docs) store the exact passed-in pointer
instead of copy the string.

While we are here we must convert sizeof on XPCOM_SEARCH_KEY and
XPCOM_ENV_PATH_SEPARATOR to strlen as well.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Callek, want to fix this too?

Comment 2

[Justin Wood (:Callek)]

Attachment: Described Patch

This patch gets rid of the static buffer which will hamper perf [slightly] and
likely cause a leak where there was none before, however this is the correct
behavior due to the details with PR_SetEnv currently, 

we may be able to revert if PR_SetEnv every does the buffer alloc internally.

Comment 3

[Justin Wood (:Callek)]

Comment on attachment 178439 [details] [diff] [review]
Described Patch

I am missing spaces between the curlys, will be in patch for checkin.

Comment 4

[Justin Wood (:Callek)]

wtc can I just get a verbal r+ on this, just to satisfy myself that my mind is
correct on the nspr use here. [If you have time]

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 178439 [details] [diff] [review]
Described Patch

>+   * This buffer will leak at Shutdown
>+   * due to a restriction in PR_SetEnv

Can you fix that wrapping, and either reference a bug number or describe the
restriction in more detail?

>+  char * tempPath = PR_smprintf(XPCOM_SEARCH_KEY "=%s" XPCOM_ENV_PATH_SEPARATOR "%s",
>+                                      grePath, path);

Second line is over-indented.

sr=shaver, but let's hear from dougt before you check in, or else wtc.

Comment 6

[Wan-Teh Chang]

Comment on attachment 178439 [details] [diff] [review]
Described Patch

This patch is correct.

The PR_smprintf code path in the original code is
wrong because it doesn't store the return value of
PR_smprintf in spEnvString.  Apparently the static
buffer sEnvString is always large enough, otherwise
we should have seen a bug.

You should not capitalize "shutdown" in the comment.

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 178439 [details] [diff] [review]
Described Patch

shaver	Callek: r+sr=shaver
shaver	Callek: please note that in the bug when you commit, so I remember
later =)

I just need to address the review nits from shaver before this is checked in.

Comment 8

[Justin Wood (:Callek)]

Attachment: Patch for checkin

Patch addressing Shavers nits, ready for comit, porting r+sr=shaver

Comment 9

[Justin Wood (:Callek)]

Attachment: Real patch for checkin

Submitted too early, sorry for bugspam.

Comment 10

[Asa Dotzler [:asa]]

Callek, can you tell us what value this adds to 1.8b2, what kind of risk it
poses, and what kind of testing's been done on the patch?

Comment 11

[Justin Wood (:Callek)]

This makes the PR_SetEnv on posix environments actually correct (I do not claim
to know what happens when it is set in the 'before' case, but the PR_SetEnv
header notes and more importantly wtc tells me that is correct).

Secondly this prevents repetitious leaks with PR_smprintf when the new path
length would be greater than MAXPATHLEN*10 (which looking at code, is not a
working code-path currently either)

Further problems with old code is that sizeof() is incorrect as used, it really
should be a strlen call. (Which is not needed in my version)

Though this code does leak (as intended and noted in comments) even if called
more than once, will always free the previous allocation.

This code has been in my tree (win32) since its inception without any problems.
 I can test tonight with a very large path if you want that tested, though with
the new code-paths it should not make a difference, (with the old it would).

I did not test what, if any, perf hit this had.

I hope this helps you Asa

Comment 12

[Benjamin Smedberg]

This is a low-risk patch that is worthy of 1.8b2.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 180440 [details] [diff] [review]
Real patch for checkin

a=caillon for 1.8beta, but please be mindful of any regressions.  We might need
to back this out if it causes problems or performance hits (though I'm not sure
why it would).

Comment 14

[Justin Wood (:Callek)]

checked in by timeless on  2005-04-21 09:48

-->Fixed