Closed
Bug 1075976
Opened 10 years ago
Closed 10 years ago
Clean up XPCOM string usage in SSLServerCertVerification.cpp
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: neil, Assigned: neil)
References
Details
Attachments
(1 file)
6.12 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8673b55bb1
Comment 6•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b80fc984aaa6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → neil
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•