Signed/unsigned warnings in expat/xmlparse/xmlparse.c

VERIFIED FIXED in M9

Status

()

Core
XML
P3
trivial
VERIFIED FIXED
19 years ago
19 years ago

People

(Reporter: Kenneth Herron, Assigned: Nisheeth Ranjan)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
Size_t is unsigned on linux, so using an int where a size_t is expected
leads to signed/unsigned warnings from gcc:

../../../mozilla/expat/xmlparse/xmlparse.c: In function
`XML_ExternalEntityParserCreate':
../../../mozilla/expat/xmlparse/xmlparse.c:585: warning: passing arg 2 of
`XML_ParserCreateNS' with different width due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c: In function `XML_Parse':
../../../mozilla/expat/xmlparse/xmlparse.c:863: warning: passing arg 1 of
`malloc' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:863: warning: passing arg 2 of
`realloc' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:873: warning: passing arg 3 of
`memcpy' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:880: warning: passing arg 3 of
`memcpy' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c: In function `XML_GetBuffer':
../../../mozilla/expat/xmlparse/xmlparse.c:911: warning: passing arg 3 of
`memmove' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:923: warning: passing arg 1 of
`malloc' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:930: warning: passing arg 3 of
`memcpy' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c: In function `doContent':
../../../mozilla/expat/xmlparse/xmlparse.c:1300: warning: passing arg 2 of
`realloc' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:1305: warning: passing arg 3 of
`memcpy' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:1327: warning: passing arg 2 of
`realloc' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c:1406: warning: passing arg 3 of
`memcmp' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c: In function `storeAtts':
../../../mozilla/expat/xmlparse/xmlparse.c:1738: warning: passing arg 2 of
`realloc' as unsigned due to prototype
../../../mozilla/expat/xmlparse/xmlparse.c: In function `addBinding':
../../../mozilla/expat/xmlparse/xmlparse.c:1760: warning: passing arg 2 of
`realloc' as unsigned due to prototype
 ../../../mozilla/expat/xmlparse/xmlparse.c: In function
`handleUnknownEncoding':
../../../mozilla/expat/xmlparse/xmlparse.c:2086: warning: passing arg 1 of
`malloc' as unsigned due to prototype

The attached patch removes all of these warnings and also fixes a possible
memory leak which had been tagged FIXME.  The "Blamed builds" page assigns
all of the above warnings to nisheeth.
(Reporter)

Comment 1

19 years ago
Created attachment 1079 [details] [diff] [review]
expat/xmlparse/xmlparse.c size_t patch
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M9
(Assignee)

Comment 2

19 years ago
Accepting bug and setting target milestone to M9...
(Assignee)

Comment 3

19 years ago
By the way, thanks a lot for this work, kherron!  :-)  You qualify as a friend
of the tree for this week!
(Assignee)

Comment 4

19 years ago
James, please take a look at the patch attached to this bug report.  I've looked
at it and it seems ok.  If you approve the patch, I'll check it in to mozilla's
copy of expat and you can update the main expat distribution.  Thanks.

Comment 5

19 years ago
I wouldn't want to include this patch in the main expat distribution.  I don't
want to use size_t in the code.  My experience is that using size_t as a
variable type tends to lead to subtle bugs, and it's better avoided (see "Large
Scale C++ Software Design" by Lakos). It seems pointless to me to enable
compiler warnings for passing an int as an argument to a function expecting an
unsigned: it's harmless, well-defined and lots of software does it. If you
really have to avoid them, it would be better to just define the functions as
macros that cast their size_t arguments to the appropriate type:

#define malloc(x) malloc((size_t)x)

(using the ANSI C anti-recursion feature). This should be done in
xmltok/xmldef.h and should be conditioned on some preprocessor symbol.

Actually I'm a bit surprised you are getting warnings about malloc, realloc:
xmldef.h defines malloc as PR_Malloc() etc when MOZILLA is defined.

The one part that is definitely a bad idea is using realloc() with a zero first
argument.  This doesn't work on older Unix systems.
(Assignee)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

19 years ago
I've used James's suggestion and added macros for memcpy, memmove, memcmp in and
changed the existing macros for malloc, realloc to get rid of the warnings.

I'm about to check in xmldef.h with the above changes.

Marking bug fixed.

Comment 7

19 years ago
If you ifdef your changes to xmldef.h on some symbol that gets defined during
MOZILLA builds, then I can fold your changes into the expat distribution.  If
they are unconditional, I can't.

I don't think

 #if PR_BYTES_PER_INT != 4
 typedef PRInt32 int;
 #endif

will compile when PR_BYTES_PER_INT != 4. You would need to do:

#ifdef PR_BYTES_PER_INT != 4
#define int PRInt32
#endif

Also if you do this, you need to ensure that xmldef.h gets included before
xmlparse.h in nsExpatTokenizer.cpp and any other clients of the expat library
(you'll probably want to undef int after including xmlparse.h).

Updated

19 years ago
QA Contact: chrisd → janc

Comment 8

19 years ago
Jan: This looks like a whitebox issue for verification. Please take a look.
Thanks

Updated

19 years ago
Whiteboard: Requesting verification from reporter via email

Comment 9

19 years ago
Requesting verification from reporter via email

Updated

19 years ago
Whiteboard: Requesting verification from reporter via email → 8/16: Requesting verification from reporter via email
(Reporter)

Updated

19 years ago
Status: RESOLVED → VERIFIED
Whiteboard: 8/16: Requesting verification from reporter via email
(Reporter)

Comment 10

19 years ago
Warnings are gone on linux.
You need to log in before you can comment on or make changes to this bug.