Closed Bug 1412646 Opened 2 years ago Closed 2 years ago

[Static Analysis] Uninitialized fields in some classes of security/manager

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

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)

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.
Attached patch security_manager.patch (obsolete) — Splinter Review
Attachment #8923165 - Flags: review?(dkeeler)
Blocks: 1411595
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)
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]
(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)
https://hg.mozilla.org/mozilla-central/rev/202ff4e53892
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: crypto-core-security → core-security-release
Please request Beta approval on this when you get a chance.
Flags: needinfo?(tristanbourvon)
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 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+
Whiteboard: [psm-assigned] → [psm-assigned][adv-main58+]
Flags: qe-verify-
Whiteboard: [psm-assigned][adv-main58+] → [psm-assigned][adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.