Last Comment Bug 307293 - Tag and type members of nsNSSASN1Sequence/nsNSSASN1PrintableItem remain uninitialized
: Tag and type members of nsNSSASN1Sequence/nsNSSASN1PrintableItem remain unini...
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-06 18:12 PDT by Simon Fraser
Modified: 2005-09-16 11:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.11 KB, patch)
2005-09-06 18:21 PDT, Simon Fraser
wtc: review+
rrelyea: superreview+
asa: approval1.8b5+
Details | Diff | Review

Description Simon Fraser 2005-09-06 18:12:39 PDT
nsNSSASN1Sequence and nsNSSASN1PrintableItem do not initialize their mType and
mTag member variables, so later calls to GetType() and GetTag() return garbage.

For some reason these are not being filled in when the objects are created.
Comment 1 Simon Fraser 2005-09-06 18:20:23 PDT
It seems that the nsNSSCertificate::CreateASN1Struct() code path does not
initialize the tag and type fields.
Comment 2 Simon Fraser 2005-09-06 18:21:26 PDT
Created attachment 195094 [details] [diff] [review]
Patch
Comment 3 Wan-Teh Chang 2005-09-07 12:00:37 PDT
Comment on attachment 195094 [details] [diff] [review]
Patch

I can't say if 0 is the right initial value for
mType and mTag.  But I verified that this patch
is correct C++ code and it does no harm.

0 is an invalid value for mTag (valid values are
0x01 - 0x1f).  For mType, 0 either means ASN1_END_CONTENTS
or is invalid.	So this patch replaces random garbage
by fixed garbage, which is an improvement.
Comment 4 Wan-Teh Chang 2005-09-07 12:09:31 PDT
The mType and mTag members are set by the
buildASN1ObjectFromDER static function in
security/manager/ssl/src/nsNSSASN1Object.cpp,
which is only called by CreateFromDER,
but no code calls CreateFromDER.

If no code (other than the constructors after the
proposed patch is applied) sets the mTag and mType
members, then we shouldn't call the GetType and
GetTag methods either.

So one can also view the mTag and mType members
as dead code and they should be removed along with
the CreateFromDER and buildASN1ObjectFromDER functions,
and the GetType, SetType, GetTag, SetTag methods.
Comment 5 Asa Dotzler [:asa] 2005-09-13 09:52:29 PDT
Simon, can you describe the user impact and the risk in taking this patch?
Comment 6 Simon Fraser 2005-09-13 11:17:53 PDT
As far as I know, there is no user impact, as I think no-one is using the
accessors for those uninitialized fields. This is simply a "correctness" fix.
Comment 7 Simon Fraser 2005-09-16 11:59:45 PDT
Fixed on trunk and branch.

Note You need to log in before you can comment on or make changes to this bug.