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

Status: VERIFIED FIXED

(Core :: XML, defect, P3)

Description

[Kenneth Herron]

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.

Comment 1

[Kenneth Herron]

Attachment: expat/xmlparse/xmlparse.c size_t patch

Comment 2

[Nisheeth Ranjan]

Accepting bug and setting target milestone to M9...

Comment 3

[Nisheeth Ranjan]

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

Comment 4

[Nisheeth Ranjan]

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

[James Clark]

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.

Comment 6

[Nisheeth Ranjan]

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

[James Clark]

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).

Comment 8

[Christine Hoffman]

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

Comment 9

[Jan Carpenter]

Requesting verification from reporter via email

Comment 10

[Kenneth Herron]

Warnings are gone on linux.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af6bc2b4498a
[components] Issue https://github.com/mozilla-mobile/android-components/issues/11063: Deprecate BrowserAwesomeBar
https://hg.mozilla.org/integration/autoland/rev/0b5b8aa4edfb
[components] Issue https://github.com/mozilla-mobile/android-components/issues/11063: samples-browser: Replace deprecated BrowserAwesomeBar with AwesomeBar composable
https://hg.mozilla.org/integration/autoland/rev/7134703a8f3c
[components] Issue https://github.com/mozilla-mobile/android-components/issues/11063: Add BrowserAwesomeBar deprecation to changelog
https://hg.mozilla.org/integration/autoland/rev/816a4d925a26
[fenix] For https://github.com/mozilla-mobile/fenix/issues/11063: Update telemetry for etp settings