229295 - lib/smime/cmsrecinfo.c: missing braces around initializer

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

When compiling lib/smime/cmsrecinfo.c, gcc issues the following
compiler warning:

gcc -o Linux2.4_x86_glibc_PTH_DBG.OBJ/cmsrecinfo.o -c -g -fPIC -DLINUX1_2 -Di386
 -D_XOPEN_SOURCE -DLINUX2_1 -ansi -Wall -pipe -DLINUX -Dlinux -D_POSIX_SOURCE -D
_BSD_SOURCE -DHAVE_STRERROR -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_wtc -D_REENTRANT
-I../../../../dist/Linux2.4_x86_glibc_PTH_DBG.OBJ/include  -I../../../../dist/pu
blic/nss -I../../../../dist/private/nss -I../../../../dist/public/dbm  cmsrecinf
o.c
cmsrecinfo.c:64: warning: missing braces around initializer
cmsrecinfo.c:64: warning: (near initialization for `fakeContent.oid')

Comment 1

[Wan-Teh Chang]

Attachment: Patch (if we still want to use an initiaizer)

An alternative fix is to remove the initializer
all together.  The compiler zeros all static
variables by default, so the initializer is
merely documentation.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 137897 [details] [diff] [review]
Patch (if we still want to use an initiaizer)

If all we want to do is silence this warning, then I'd say just
get rid of the initializer.  In addition to silencing the warning, 
it would move the variable (a structure) from the initialized data segment to
BSS, reducing the size of the binaries.  

But IMO, this warning is telling us about the tip of an iceberg, 
and rather that silence the warning, we should destroy the 
iceberg.  I'd prefer to leave this warning unsilenced until we're ready to
destroy this iceberg.

First, the address of this global struct is used to initialize 
a variable named contentTypeTag in the ContentInfo struct.
That variable is seriously misnamed, because it is not a 
SecOidTag at all, but rather a pointer to a SECOidData struct.

Second, the "fakeContent" SECOidData struct is not properly 
initialized for a SECOidData struct.  Being initialized to all 
zeros, it appears to be a secoiddata struct with the tag number zero, which is
SEC_OID_UNKNOWN, yet this is NOT the correct secoidData for SEC_OID_UNKNOWN. 
See http://lxr.mozilla.org/mozilla/source/security/nss/lib/util/secoid.c#534
for the proper SECOidData for SEC_OID_UNKNOWN.	

Third, the address of members of this single global struct are returned by
various methods on the ContentInfo object, such as 
NSS_CMSContentInfo_GetContentTypeOID and the addresses returned 
are not pointers to "const", so it would be easy for code to modify this single
global instance.  

It apparently was the author's intent that the address of this "fakeContent"
global instance be a "magic" number, used to signify 
that this SECOidData was not an ordinary one.  Yet nothing is done
to prevent the other ContentInfo methods from treating it like an
ordinary SECOidData.  For example, the methods
NSS_CMSContentInfo_GetContentTypeTag and NSS_CMSContentInfo_GetContentTypeOID
do not recognize this address
and treat it specially.  

A number of questions should probably be answred before attempting
to fix this.

a) Why is contentInfo.contentTypeTag a SECOidData * rather than a 
SECOidTag?  Wouldn't a "magic" tag value be better than a "magic"
structure address?

b) If fakeContent is intended to have different semantics than
SEC_OID_UNKNOWN, why is it initialized with the tag value SEC_OID_UNKNOWN? 
What other tag value SHOULD it be given?

c) If fakeContent is intended to have the same semantics as 
SEC_OID_UNKNOWN, then why use a separate global isntance? 
Why not use the address of the official SEC_OID_UNKNOWN SECOidData (e.g. the
address returned by SECOID_FindOIDByTag(SEC_OID_UNKNOWN) )?

d) If fakeContent is intended never to be modified, then why isn't
it const, as the official SECOidData structs are in
http://lxr.mozilla.org/mozilla/source/security/nss/lib/util/secoid.c#534
If it were const, then attempts to modify it would result in 
crashes, at least on some platforms.  

Let's not silence this warning until the iceberg is history.

Comment 3

[Robert Relyea]

The code looked rather familiar to me, so I did a little digging into the
history of this patch.

It was triggered by bug 192590. It primarily is driven by the need to keep
binary compatiblility since the returned data structures were not opaque (sigh).
With this info I'll attempt to answer nelson's questions:

> a) Why is contentInfo.contentTypeTag a SECOidData * rather than a 
> SECOidTag?  Wouldn't a "magic" tag value be better than a "magic"
> structure address?

The effective question is why can't we change SECOidData * to a SECOidTag? The
answer is the data structure is not opaque and can't be modified without
breaking binary compatibility.

> b) If fakeContent is intended to have different semantics than
> SEC_OID_UNKNOWN, why is it initialized with the tag value SEC_OID_UNKNOWN? 
> What other tag value SHOULD it be given?

The contentInfo itself isn't valid. The ContentTypeTag was is simply a way of
marking that. If you pass this contentInfo to any other parts of the CMS code,
weird things would happen anyway.

> c) If fakeContent is intended to have the same semantics as 
> SEC_OID_UNKNOWN, then why use a separate global isntance? 
> Why not use the address of the official SEC_OID_UNKNOWN SECOidData (e.g. the
> address returned by SECOID_FindOIDByTag(SEC_OID_UNKNOWN) )?

FakeContent simply means the content field isn't a normal valid content field.
It needs to be a value you *wouldn't* see coming out of the ASN.1 decoder,
otherwise the code will assume that a valid contentInfo would be and one of our
face ones. 

If we weren't tied to binary compatibility, we would have a new typed data
structure that couldn't be confused with a contentInfo.

> d) If fakeContent is intended never to be modified, then why isn't
> it const, as the official SECOidData structs are in
> http://lxr.mozilla.org/mozilla/source/security/nss/lib/util/secoid.c#534
> If it were const, then attempts to modify it would result in 
> crashes, at least on some platforms. 

Because I habitually underutilize const. This data structure should be labelled
'const'.

> Let's not silence this warning until the iceberg is history.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I disagree that this structure is not opaque.  The fact that the structure
declaration appears in open source does not mean it is not opaque.  
The object type has appropriate accessors.  It is opaque by definition,
that is, by fiat of the author.  Let's discuss this at today's meeting.

Comment 5

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 6

[:Cykesiopka]

It looks like this was fixed as part of Bug 272484:
https://hg.mozilla.org/projects/nss/rev/4301f1b8fadd#l8.13

If this is incorrect, please feel free to re-open.