1268365 - Check argument validity more in nsASN1Tree.cpp

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

Many of the functions in nsASN1Tree.cpp don't have checks that ensure their arguments are valid. Checks should be added to ensure callers aren't doing the wrong thing.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1268365 - Check argument validity more in nsASN1Tree.cpp. r=jcj

Review commit: https://reviewboard.mozilla.org/r/49635/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49635/

Comment 2

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8746944 [details]
MozReview Request: Bug 1268365 - Check argument validity more in nsASN1Tree.cpp. r=jcj

https://reviewboard.mozilla.org/r/49635/#review46533

I'm going to redirect to JC for this review. Also, I wouldn't spend too much effort on fixing up the nsASN1* code or any of the C++ code that implements the certificate manager/viewer, since it should pretty much all be rewritten in JS anyway (which is something I'm hoping will be possible in the nearish future).

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8746944 [details]
MozReview Request: Bug 1268365 - Check argument validity more in nsASN1Tree.cpp. r=jcj

https://reviewboard.mozilla.org/r/49635/#review46749

Thank you for doing this cleanup, Cykesiopka! This looks good to me.

Comment 4

[:Cykesiopka]

Comment on attachment 8746944 [details]
MozReview Request: Bug 1268365 - Check argument validity more in nsASN1Tree.cpp. r=jcj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49635/diff/1-2/

Comment 5

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> Also, I wouldn't spend too much
> effort on fixing up the nsASN1* code or any of the C++ code that implements
> the certificate manager/viewer, since it should pretty much all be rewritten
> in JS anyway (which is something I'm hoping will be possible in the nearish
> future).

Oh, cool! Good to know.

Comment 6

[:Cykesiopka]

Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d231627c8a457e731dcad0a48f3436424bddf9c
(Ignore the Android checkstyle failure - nothing in the patch touches anything that would trigger such failures).

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4c042c6d3a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f4c042c6d3a