strict aliasing bugs in mozilla/dbm

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P3
trivial
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

unspecified
3.12
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

15 years ago
(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
(Reporter)

Comment 1

15 years ago
Created attachment 103569 [details] [diff] [review]
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?
(Reporter)

Comment 2

15 years ago
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.
Status: NEW → ASSIGNED
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.
(Reporter)

Comment 4

15 years ago
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.
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

15 years ago
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

15 years ago
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

15 years ago
> 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.
(Reporter)

Comment 9

15 years ago
Created attachment 104203 [details] [diff] [review]
eliminate all necko references to dbm

Comment 10

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

thx bryner! sr=darin
Attachment #104203 - Flags: superreview+
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 :)
Attachment #104203 - Flags: review+
(Reporter)

Comment 12

15 years ago
Created attachment 104300 [details] [diff] [review]
also remove dbm from netwerk's search path on mac

Comment 13

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

sr=darin
Attachment #104300 - Flags: superreview+
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)
Attachment #104300 - Flags: review+
Comment on attachment 104300 [details] [diff] [review]
also remove dbm from netwerk's search path on mac

a=roc+moz for trunk
Attachment #104300 - Flags: approval+
(Reporter)

Comment 16

15 years ago
Checked in the patch.  Leaving the bug open.
(Assignee)

Comment 17

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

Comment 18

14 years ago
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

14 years ago
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.
Product: Browser → Seamonkey
(Reporter)

Updated

11 years ago
Assignee: bryner → general
Status: ASSIGNED → NEW
QA Contact: asa → general

Updated

11 years ago
Assignee: general → nobody
Component: General → General
Product: Mozilla Application Suite → Core
QA Contact: general → general

Comment 20

11 years ago
The NSS team now owns the DBM module.  Let's do what Nelson
suggested in comment 18.
Assignee: nobody → nobody
Component: General → Libraries
Product: Core → NSS
QA Contact: general → libraries
Version: Trunk → unspecified
(Assignee)

Updated

11 years ago
Assignee: nobody → nelson
(Assignee)

Comment 21

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 22

11 years ago
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.
Severity: normal → trivial
Status: RESOLVED → REOPENED
Priority: -- → P3
Resolution: FIXED → ---
Target Milestone: --- → 3.12
(Assignee)

Comment 23

11 years ago
Created attachment 227490 [details] [diff] [review]
remove ndbm.* from manifest.mn files
Attachment #227490 - Flags: review?(wtchang)

Comment 24

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

r=wtc.
Attachment #227490 - Flags: review?(wtchang) → review+
(Assignee)

Comment 25

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.