Closed Bug 1075976 Opened 5 years ago Closed 5 years ago

Clean up XPCOM string usage in SSLServerCertVerification.cpp

Categories

(Core :: Security: PSM, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file)

I happened to notice some suboptimal string patterns in this file.

In CheckCertOverrides():
* using AppendASCII to copy a string, forcing the string code to call strlen on the string, when the info object already knows the length
* making a shallow copy of the host name instead of passing the original reference (although at least it wasn't a deep copy)

In TryMatchingWildcardSubjectAltName():
* possibly not knowing about StringEndsWith (which is understandable, since it's not well documented) and using Find instead (I guess it was easy to find...)

In GatherBaselineRequirementsTelemetry
* assigning to a dependent string (which is confusing)
* manually calling strlen where string code can do that for you
Attached patch Possible patchSplinter Review
Comment on attachment 8498419 [details] [diff] [review]
Possible patch

>-  nsCString hostWithPortString;
>-  hostWithPortString.AppendASCII(mInfoObject->GetHostNameRaw());
>+  nsAutoCString hostWithPortString(mInfoObject->GetHostName());
>   hostWithPortString.Append(':');
>   hostWithPortString.AppendInt(port);
Since this is a transient variable, an nsAutoCString makes more sense, as the characters can be collected in its local buffer space.

>-      nsCString hostString(mInfoObject->GetHostName());
>+      const nsACString& hostString(mInfoObject->GetHostName());
>       nsrv = overrideService->HasMatchingOverride(hostString, port,
The line was too long for me to consider inlining the single use of the variable.

>-    nsDependentCString altName;
>+    nsAutoCString altName;
I couldn't tell from reading the code whether currentName->name.other.data is known to be null-terminated; if it is, then it would be possible to stick with a dependent string and just rebind it as appropriate.

>-      nsDependentCString altNameWithoutWildcard(altName);
>-      if (altNameWithoutWildcard.Find("*.") == 0) {
>-        altNameWithoutWildcard.Assign(altName.get() + 2, altName.Length() - 2);
>+      nsDependentCString altNameWithoutWildcard(altName, 0);
>+      if (StringBeginsWith(altNameWithoutWildcard, NS_LITERAL_CSTRING("*."))) {
>+        altNameWithoutWildcard.Rebind(altName, 2);
>         commonNameInSubjectAltNames |=
>           TryMatchingWildcardSubjectAltName(commonName.get(), altName);
>       }
>       // net_IsValidHostName appears to return true for valid IP addresses,
>       // which would be invalid for a DNS name.
>       // Note that the net_IsValidHostName check will catch things like
>       // "a.*.example.com".
>       if (!net_IsValidHostName(altNameWithoutWildcard) ||
Another approach here would be to have a variable which is either 0 or 2 depending on whether the altName includes a wildcard, and pass Substring(altName, chopWildcard) to net_IsValidHostName.
Attachment #8498419 - Flags: feedback?(dkeeler)
Comment on attachment 8498419 [details] [diff] [review]
Possible patch

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

Looks great, Neil. I appreciate you taking the time to do this. Now hopefully I'm more likely to do the efficient/correct thing the next time I need to deal with strings. This is basically ready to go, so r=me.
Attachment #8498419 - Flags: feedback?(dkeeler) → review+
(In reply to neil@parkwaycc.co.uk from comment #2)
> >-    nsDependentCString altName;
> >+    nsAutoCString altName;
> I couldn't tell from reading the code whether currentName->name.other.data
> is known to be null-terminated; if it is, then it would be possible to stick
> with a dependent string and just rebind it as appropriate.

I believe currentName->name.other.data isn't guaranteed to be null-terminated, so looks like this is the safe way to go.
http://hg.mozilla.org/mozilla-central/rev/b80fc984aaa6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → neil
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.