423401 - Please use a local variable and copy the string once

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[timeless]

the current code does pointer work on a string which should be local until it's done.

it's also doing Append the first time instead of some flavor of Assign. nsPrintfCString might be useful...

Comment 1

[Kai Engert [:KaiE:]]

This bug report currently depends on cvs-blame to work.
I'm pasting the relevant information, so we know which code this refers to:

in SECStatus nsNSSHttpRequestSession::createFcn

  rs->mURL.Append(nsDependentCString(http_protocol_variant));
  rs->mURL.AppendLiteral("://");
  rs->mURL.Append(hss->mHost);
  rs->mURL.AppendLiteral(":");
  rs->mURL.AppendInt(hss->mPort);
  rs->mURL.Append(path_and_query_string);

Comment 2

[Edmund Wong (:ewong)]

Attachment: Used a local variable to gather the information and then it is assigned to the url.

Comment 3

[Edmund Wong (:ewong)]

Attachment: Used a local variable to gather the information and then it is assigned to the url. (v2)

Cleaned the code as suggested by timeless via IRC.

Comment 4

[[e-mail]]

Comment on attachment 459736 [details] [diff] [review]
Used a local variable to gather the information and then it is assigned to the url.

>+  int slen = 16 + strlen(http_protocol_variant) +
>+             nsPromiseFlatCString(hss->mHost).Length()+
>+             strlen(path_and_query_string);
Any reason not to use strlen on hss->mHost?

>+                                        nsPromiseFlatCString(hss->mHost).get(),
This is almost the same as hss->mHost, but much more work.

Also you're not supposed to use nsPromiseFlatCString directly.

Comment 5

[Edmund Wong (:ewong)]

(In reply to comment #4)
> Comment on attachment 459736 [details] [diff] [review]
> Used a local variable to gather the information and then it is assigned to the
> url.
> 
> >+  int slen = 16 + strlen(http_protocol_variant) +
> >+             nsPromiseFlatCString(hss->mHost).Length()+
> >+             strlen(path_and_query_string);
> Any reason not to use strlen on hss->mHost?
> 
I had an issue with doing strlen(hss->mHost).  Gave me the
following error:

nsNSSCallbacks.cpp(241) : error C2664: 'strlen' : cannot convert parameter 1 from 'nsCString' to 'const char *'


> >+                                        nsPromiseFlatCString(hss->mHost).get(),
> This is almost the same as hss->mHost, but much more work.
> 
> Also you're not supposed to use nsPromiseFlatCString directly.

Is the second patch better?  It doesn't call nsPromiseFlatCString
directly.

Comment 6

[Kai Engert [:KaiE:]]

I don't understand why there is a need to change the nice and safe object oriented string approach to an old-world C-style approach, that requires the introduction of arbitrary numbers like "16" into the code, which readers will have trouble to understand. Why 16?

From the comment of nsPrintfCString: 
"If your formatted string exceeds the allocated space, it is cut off at the size of the buffer, no error is reported"

Yes, it's probably unlikely that in the future the port number will be a 64 bit number that has a very long result and will be longer than 12 chars, but who knows?

I'm not in favour of changing code that currently deals correctly with any length of numbers into code that behaves wrong for some numbers.


I have trouble to understand the motivation for this bug report.

The initial comment should have been written more clearly.

What exactly do you mean with "pointer work on a string which should be local until it's done." and to which of the lines are you referring to?

Please clarify that.


Is is that you're worried about the combination of "Append" with "nsDependentCString" in a single statement, which might introduce a pointer to a temporary object? If you want to clear that up, fair enough, but there is no need to rewrite the whole code simply for this fix. Simply move the nsDependentCString definition to an earlier separate line, so it's no longer a temporary.

On the other hand, if you look at the code, both http_protocol_variant and path_and_query_string and simply "const char *". It seems inconsistent why the current code uses different treatment (once a direct append, once an append using the nsDep...).

I propose to use the same style for both. It looks like nsDep. can be avoided at all, so the above split is not even necessary (simply remove nsDep.)


If you're worried about the lack of initial assignment, well, I personally thing it's not a big deal. Most likely the construction of an empty string object won't involve a memory allocation at all, so I don't see an advantage of changing it to use an initial assignment.

If you're really that worried about multiple allocations, then you can use rs->mURL.SetCapacity(expected-maximum-value).

Comment 7

[Edmund Wong (:ewong)]

Attachment: Removed nsDependentCString() usage.

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 461129 [details] [diff] [review]
Removed nsDependentCString() usage.

r=kaie

Comment 9

[Edmund Wong (:ewong)]

Attachment: Removed nsDependentCString() usage.

Changed patch to be more checkin-needed friendly.
Bug 423401 - Removed nsDependentCString() usage. r=kaie, r=honzab.moz

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bc0f563bf695