Closed
Bug 1412646
Opened 7 years ago
Closed 7 years ago
[Static Analysis] Uninitialized fields in some classes of security/manager
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: tristanbourvon, Assigned: tristanbourvon)
References
Details
(Keywords: csectype-uninitialized, sec-moderate, Whiteboard: [psm-assigned][adv-main58+][post-critsmash-triage])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
keeler
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
As part of the static analysis effort to ensure all class fields are initialized before usage (see https://bugzilla.mozilla.org/show_bug.cgi?id=525063), here are a few fixes to the security/manager module.
Updated•7 years ago
|
Assignee: nobody → tristanbourvon
Group: core-security → crypto-core-security
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8923165 [details] [diff] [review] security_manager.patch Review of attachment 8923165 [details] [diff] [review]: ----------------------------------------------------------------- Hi Tristan - thanks for the patch. Please re-submit it with 8 lines of unified context and following the style guidelines at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style (in particular, the "C++ classes" section, which illustrates how we format initialization lists). See also https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions ::: security/manager/ssl/DataStorage.cpp @@ +73,3 @@ > DataStorage::DataStorage(const nsString& aFilename) > : mMutex("DataStorage::mMutex") > + , mTimerDelay{0}, mPendingWrite(false) This should be sDataStorageDefaultTimerDelay. Also, let's use value initialization (i.e. "()" instead of "{}"). Additionally, these each need to be on their own line. ::: security/manager/ssl/ScopedNSSTypes.h @@ +109,4 @@ > { > public: > Digest() > + : mItemBuf() { nit: '{' on its own line
Attachment #8923165 -
Flags: review?(dkeeler)
Assignee | ||
Comment 3•7 years ago
|
||
Sorry about the previous low quality patch, we have such a huge amount of patches for this type of issue that I tried to rush things and forgot about code standards. Hope this is better.
Attachment #8923165 -
Attachment is obsolete: true
Attachment #8923762 -
Flags: review?(dkeeler)
Comment on attachment 8923762 [details] [diff] [review] security_manager.patch Review of attachment 8923762 [details] [diff] [review]: ----------------------------------------------------------------- This is great - thanks! (It would be best to have 8 lines of unified context, but in this case I imagine we should be fine.) For posterity, I was intending my tone in comment 2 to be informative and straightforward, but perhaps I was too stern. I appreciate you taking the time to fix this bug.
Attachment #8923762 -
Flags: review?(dkeeler) → review+
Priority: -- → P1
Whiteboard: [psm-assigned]
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4) > Comment on attachment 8923762 [details] [diff] [review] > security_manager.patch > > Review of attachment 8923762 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is great - thanks! > (It would be best to have 8 lines of unified context, but in this case I > imagine we should be fine.) > For posterity, I was intending my tone in comment 2 to be informative and > straightforward, but perhaps I was too stern. I appreciate you taking the > time to fix this bug. Don't worry, I didn't take it as something else than a good review on a neutral tone. As for the unified context, I'm not sure I understand what's wrong... I see 4 lines of context before and after each change, doesn't that make 8 lines of context? Or maybe you meant 8 on both sides?
Flags: needinfo?(dkeeler)
Yes - 8 on both sides. Your .hgrc (or equivalent) should have something like this: [diff] git = 1 showfunc = 1 unified = 8 [defaults] diff = -p -U 8 (I don't actually know if that's redundant - they say almost the same thing.)
Flags: needinfo?(dkeeler)
Updated•7 years ago
|
Keywords: csectype-uninitialized,
sec-moderate
![]() |
||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/202ff4e53892
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
Group: crypto-core-security → core-security-release
Comment 10•7 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → wontfix
Flags: needinfo?(tristanbourvon)
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8923762 [details] [diff] [review] security_manager.patch Approval Request Comment [Feature/Bug causing the regression]: Uninitialized members [User impact if declined]: Possible (unconfirmed) security issues [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Builds and passes tests, no noticeable perf regression [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: Low risk [Why is the change risky/not risky?]: Only some specific code relying on UB would be affected by this change, and tests seem to indicate that no problems have been introduced [String changes made/needed]: N/A
Flags: needinfo?(tristanbourvon)
Attachment #8923762 -
Flags: approval-mozilla-beta?
Comment 12•7 years ago
|
||
Comment on attachment 8923762 [details] [diff] [review] security_manager.patch 2 weeks in nightly, let's try this on beta.
Attachment #8923762 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [psm-assigned] → [psm-assigned][adv-main58+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [psm-assigned][adv-main58+] → [psm-assigned][adv-main58+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•