175741 - strict aliasing bugs in mozilla/dbm

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Brian Ryner (not reading)]

(I can't believe there's no component for dbm! cc'ing a few people listed as
owners/peers on owners.html)

cvs trunk builds of gcc spew out a lot of warnings about strict aliasing
problems in mozilla/dbm/ndbm.c.  We should probably fix these since experience
says (see bug 83388, also bug 165605, caused by bug 175668) that these cause
legitimate problems.

ndbm.c: In function `dbm_fetch':
ndbm.c:104: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c:104: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c: In function `dbm_firstkey':
ndbm.c:123: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c:123: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c: In function `dbm_nextkey':
ndbm.c:140: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c:140: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c: In function `dbm_delete':
ndbm.c:155: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c: In function `dbm_store':
ndbm.c:171: warning: dereferencing type-punned pointer will break
strict-aliasing rules
ndbm.c:171: warning: dereferencing type-punned pointer will break
strict-aliasing rules

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This quiets the warnings. Maybe someone can comment on whether I'm just
papering over the problem, or whether there was a problem to begin with?

Comment 2

[Brian Ryner (not reading)]

Thinking about this some more, I think these particular warnings are harmless. 
Strict aliasing would be a problem if a store to a particular memory location
could be cached in a register with the assumption that the memory location would
not be dereferenced as a different type (as was happening in the jsdtoa strict
aliasing bug).  I'm fairly sure it's not possible for that type of caching to
happen across a function return, since the calling convention doesn't provide
for it.  Similarly, passing a pointer to a struct as a function parameter would
seem to require that stores to that struct be flushed to memory first.  So,
notwithstanding inline functions and fancy cross-function optimization within
the same source file, I think the only time this could be a problem is when a
struct is written to and/or read with two different pointer types within the
same function.  That is not happening here.  Someone tell me if I'm out in left
field.

I still think we should fix the warnings to make it easier to spot real problems.

Comment 3

[Bradley Baetz (:bbaetz)]

No, this is a real bug, and if g++ doesn't give a warning with your patches,
then it just means that its optimsation pass isn't (yet) smart enough to notice
this.

We have:

typedef struct {
        char *dptr;
        int dsize;
} datum;

(from ndbm.h)

and

typedef struct {
        void    *data;                  /* data */
        size_t   size;                  /* data length */
} DBT;

(from mcom_db.h)

Now, the code in question boils down to:

datum retval;
DBT* dbt = (DBT*)&retval;
dbt->data = 0xdeadbeef
dbt->size = 1234;
return retval;

Firstly, these two structs are not equivalent. sizeof(size_t) != sizeof(int), so
on an LE machine where those differ, setting size via DBT will get an incorrect
result, even for a size < 2^32.  On BE machines, setting the 64 bit size_t value
will run off the end of the struct created as a datam, and could theoretically
cause a crash.

Since gcc doesn't do global program analysis, this is probaby safe from an
aliasing point of view, though. If the code then read the value as a |datum|
before returning, this would be an issue, though.

Why do we do all this casting?

Who uses dbm, and can we update to a newer version, or will taht cause problems
reading old dbs?

With your patch, if you try to read via datam (which would be a problem), does
gcc give you a warning? If not, I'd prefer that we don't fix this, rather than
wallpapering over this problem.

Comment 4

[Brian Ryner (not reading)]

If it's safe from an aliasing point of view, as you said, I'd prefer not to see
these warnings.  I'll test what happens if i read via 'datum' when I have a chance.

Comment 5

[Bradley Baetz (:bbaetz)]

so, grepping my entire tree's Makefile.ins for |dbm|, I get:

netwerk/test/TestDBMAcess.cpp uses it - not sure why thats in necko

netwerk/build/ links against it - no clue why; maybe the cache used to use it or
something - darin?

security/manager includes it in their Makefile.in includes (via -I), and also
copies it from $(LIB_PREFIX)mozdbm_s.$(LIB_SUFFIX) to
$(LIB_PREFIX)dbm.$(LIB_SUFFIX).

nss uses it, but that seems to be it. I think its just for teh certdb, right kaie?

Note that this aliasing problem appears to be present in the db 1.85 source,
although there they use a per-os ndbm.h header file instead of our mcom_db.h.

It doesn't look like we can update, though - not only has the on disk data
format changed (although conversion utilities are provided) but sleepycat's db
stuff is under a license where the entire program which uses it must be open
source, and the GNU gdbm stuff (again, a different format) is GPL, rather than LGPL

Comment 6

[Darin Fisher]

any necko references to DBM can be eliminated.  TestDBMAccess is definitely
useless at this point and shouldn't be part of the build.

Comment 7

[gordon]

Right, TestDBMAccess was used to evaluate dbm early in the design stage of the
nsCacheService, and should *not* be part of the build (really it should never
have been part of the build).  I don't care if the source is sitting in the
directory, but we shouldn't spend any time building it, and we certainly don't
want any dependencies on DBM.  The old cache used it, but we don't anymore.

Comment 8

[Kai Engert [:KaiE:]]

> nss uses it, but that seems to be it. I think its just for teh certdb, right kaie?

Right, only NSS uses it. PSM does not directly use, only indirectly, since it
links against NSS.

Comment 9

[Brian Ryner (not reading)]

Attachment: eliminate all necko references to dbm

Comment 10

[Darin Fisher]

Comment on attachment 104203 [details] [diff] [review]
eliminate all necko references to dbm

thx bryner! sr=darin

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 104203 [details] [diff] [review]
eliminate all necko references to dbm

r=bbaetz, assuming that mac doesn't need to be changed.

Not that this has anything to do with the current bug, mind you :)

Comment 12

[Brian Ryner (not reading)]

Attachment: also remove dbm from netwerk's search path on mac

Comment 13

[Darin Fisher]

Comment on attachment 104300 [details] [diff] [review]
also remove dbm from netwerk's search path on mac

sr=darin

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 104300 [details] [diff] [review]
also remove dbm from netwerk's search path on mac

Looks right. r=bbaetz

Is it worth filing another bug to move the dbm packaging/build order/etc to nss
builds only? (ie don't build it for --disable-crypto, and so on)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 104300 [details] [diff] [review]
also remove dbm from netwerk's search path on mac

a=roc+moz for trunk

Comment 16

[Brian Ryner (not reading)]

Checked in the patch.  Leaving the bug open.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Brian, Wan-Teh,
Now that DBM is used only by NSS, any objections to changing this bug to be
product=NSS, component=libraries ??

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Now that we have established that only NSS uses DBM, I propose that we 
(NSS team) cvs remove ndbm.c from the mozilla/dbm/src directory, and 
resolve this bug WONTFIX.  Here's why:

IINM, NSS does not call any of the functions in ndbm.c.  It uses the older
db API, as implemented in mozilla/dbm/src/db.c, exclusively.  E.g. NSS
calls dbopen, not dbm_open, to access dbm.  So, the problems with ndbm.c 
reported in this bug just aren't relevant to NSS.

Any objections to this plan?

Comment 19

[Wan-Teh Chang]

I agree we should cvs remove ndbm.c and ndbm.h and mark
this bug WONTFIX.  On 64-bit platforms, "datum" and "DBT"
are not isomorphic, so the typecasts are bugs.

Regarding changing the product/component of this bug,
I'd be more comfortable after we have removed the Mozilla
makefiles in mozilla/dbm to make sure no code other than
NSS can possibly use mozilla/dbm.  Alternatively we can
add a new "mozilla/dbm" component to the "Mozilla" product
in Bugzilla.

Comment 20

[Wan-Teh Chang]

The NSS team now owns the DBM module.  Let's do what Nelson
suggested in comment 18.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

cvs update: include/nsres.h is no longer in the repository
cvs update: src/nsres.c is no longer in the repository

I cvs removed these two files on date: 2006/05/13 01:28:45 UTC

Comment 22

[Wan-Teh Chang]

Nelson, the files that need to be cvs removed are:
mozilla/dbm/include/ndbm.h
mozilla/dbm/src/ndbm.c

We also need to remove references to these two files
from mozilla/security/dbm/include/manifest.mn and
mozilla/security/dbm/src/manifest.mn.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Attachment: remove ndbm.* from manifest.mn files

Comment 24

[Wan-Teh Chang]

Comment on attachment 227490 [details] [diff] [review]
remove ndbm.* from manifest.mn files

r=wtc.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Removing dbm/include/ndbm.h; new revision: delete; previous revision: 3.2
Removing dbm/src/ndbm.c;     new revision: delete; previous revision: 3.5
security/dbm/include/manifest.mn; new revision: 1.4; previous revision: 1.3
security/dbm/src/manifest.mn;     new revision: 1.6; previous revision: 1.5