cppcheck error found and gcc warnings

[GoogleCodeExporter]

Paul Wise (Debian Developer) advices me in the last uploaded deb (1.1~r230) 
which there are some errors/warnings in the source that has been reported by 
the "cppcheck" and "gcc"

cppcheck:
===

$ cd src
$ cppcheck -j --quiet -f .
[json.c:1189]: (error) Buffer access out-of-bounds

src/json.c:

1: ...
2: ...
...
1189: sprintf (buffer, "\\u%4.4x", text[i]);
...

It try to write the string of size 6 to buffer[6] <= forgot the space for '\0'.
Therefore the buffer should declare with size of 7 instead. (The proposed patch 
"json.diff" is attached).

gcc warnings:
===
$ make 2>&1 | grep -i warn
src/check.c:669:18: warning: ‘on_metadata’ may be used uninitialized in 
this function [-Wuninitialized]
src/json.c:1337:7: warning: format ‘%llX’ expects argument of type ‘long 
long unsigned int’, but argument 3 has type ‘int64_t’ [-Wformat]
src/json.c:2806:12: warning: variable ‘temp’ set but not used 
[-Wunused-but-set-variable]

I have proposed the patch only for the first and second warnings but the last 
one I saw that you have noticed the "TODO" comment which you may add some 
implementations later. (suppress-gcc-warnings.diff)

The first warning, add an initial value for 'on_metadata' to suppress the gcc 
warning.
The second warning, casting the value which "%llX" value should be type 
unsigned.

Cheers,
Neutron Soutmun

Original issue reported on code.google.com by neo.neut...@gmail.com on 25 Jun 2011 at 4:24

Attachments:

• json.diff
• suppress-gcc-warnings.diff

[GoogleCodeExporter]

Hi,

the first warning is just the symptom of an unfinished feature (metadata 
checking) from what I call an "unclean" commit, and my currently checked-out 
version of check.c fixes this. It is nonetheless a good thing to point out, as 
I'm usually adamant to commit "clean builds" as often as possible, even in the 
unstable trunk (that makes ponder more and more to use git exclusively, since 
it makes it easier to work in feature branches than svn).

To make things clear about json.c, it is not code that I wrote, but it is 
instead the inclusion of two files from the MJSON project 
(http://sourceforge.net/projects/mjson/).

I think it would be interesting to report these issues to the upstream 
maintainer of mjson.

As I did a few patches to mjson myself, I guess we can propose their inclusion 
in the  project itself, if the latest 1.3 release which I haven't reviewed yet 
does not include equivalent bug fixes.

Anyways, I'll merge these patches ASAP.

Regards !
Marc

Original comment by marc.noi...@gmail.com on 26 Jun 2011 at 9:59

• Changed state: Started

[GoogleCodeExporter]

Original comment by marc.noi...@gmail.com on 26 Jun 2011 at 10:00

• Added labels: Milestone-1.1

[GoogleCodeExporter]

Seems like the cppcheck error has been solved upstream:

http://mjson.svn.sourceforge.net/viewvc/mjson/trunk/src/json.c?r1=158&r2=159&pat
hrev=159

Original comment by marc.noi...@gmail.com on 26 Jun 2011 at 11:12

[GoogleCodeExporter]

That's sounds good.

Paul Wise also advices me consider to package the mjson which flvmeta
could link against libs in the system.

flvmeta has included the libyaml and mjson source in the source tree.
In Debian, we should link against libs in the system
as much as possible, if not, should send them to the debian-security
team to review the source.

I'm considering to build deb and link against the existing libyaml in
Debian system but for mjson should wait until someone
package it or I have time to package it :).

Original comment by neo.neut...@gmail.com on 27 Jun 2011 at 3:14

[GoogleCodeExporter]

I have been thinking the same, it really makes sense to use existing system 
libraries.

My course of action will be to keep libyaml and mjson in the source tree (for 
convenience, and for Windows users), but my configure scripts will first check 
for an existing library on the system to link against.

By the way, what does the Debian staff think about using CMake instead of 
autotools to compile programs ? :)

Original comment by marc.noi...@gmail.com on 27 Jun 2011 at 8:58

[GoogleCodeExporter]

All patches have been applied (sometimes with modifications from your original 
patch).

Original comment by marc.noi...@gmail.com on 27 Jun 2011 at 5:06

• Changed state: Fixed

[GoogleCodeExporter]

:)

Good, if your configure scripts check the libs in the system is better.

Only autotools I have used but I heard CMake for a long long time :)
I'm a new maintainer, just starting with the debhelper and if the
advanced topic to figure out, I'm not sure that
I could still handle it :)

Original comment by neo.neut...@gmail.com on 28 Jun 2011 at 11:10

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

As the recent Debian package uploaded, I found a warning occurred again. 
Perhaps, some changes in libc that cause the uint64_t and unsigned long long 
int are not the same anymore in the 64bits architecture. (/me: Debian Sid - 
amd64).

Therefore, I have prepared an update patch which now exactly casting it to 
"unsigned long long" instead.

Please review.

Original comment by neo.neut...@gmail.com on 23 Nov 2011 at 4:49

Attachments:

• update-json-compiler-warning.diff

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Update:
Paul Wise advices me to do the better patch by implement the PRIX64 instead 
which no need to casting anymore.

See: http://lists.debian.org/debian-mentors/2011/11/msg00513.html

Original comment by neo.neut...@gmail.com on 23 Nov 2011 at 5:28

Attachments:

• update-json-compiler-warning-prix64.diff

[GoogleCodeExporter]

I'm afraid these PRI* macros are C99 specific (inttypes.h), and not supported 
by the Microsoft compilers.

I don't think I can reasonably include this patch upstream since Windows 
compatibility has been one of my objectives from start, and I'd have to do 
further modifications to json.c to ensure it still compiles on Windows.

It's not like I mind adding such a compatibility layer, but since it's in a bit 
of code (mjson) that should be fixed upstream-er, the solution I'm actually 
favoring is to completely ditch mjson, seeing as its code is not of the highest 
quality, and quite heavily patched by me already... (to the point that the 
author, Rui Maciel, himself has taken the approach of restarting the project 
from scratch in a new branch)

There are a few alternatives available (jansson, cJSON...) which I'm going to 
evaluate as replacement solutions as soon as possible.

Original comment by marc.noi...@gmail.com on 29 Nov 2011 at 12:01

• Changed state: Started
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Sure, I'm understanding your point. At the time I post this patch then I have 
worried about build the software in Windows but I have no any experiences for 
the Microsoft compilers.

I'm not sure, Is it also possible to build the flvmeta with MinGW ?
I heard that MinGW may build the gcc sources for Windows.

Original comment by neo.neut...@gmail.com on 30 Nov 2011 at 4:47

[GoogleCodeExporter]

flvmeta builds fine with Mingw32, it's easy to test since I'm using CMake as a 
build system.

However, I don't want to abandon Visual Studio as a development environment, if 
just for the comfort and for the great debugging tools, as well as 64 bit 
support (not sure if cmake has any support for the 64 bit version of mingw).

For the moment, I think it's okay to use your patch for the Debian build, 
hopefully, it won't be necessary anymore when I can successfully switch to a 
better json library.

Original comment by marc.noi...@gmail.com on 30 Nov 2011 at 10:27

[GoogleCodeExporter]

This issue is irrelevant as of 1.1.0, as I have ditched mjson altogether in 
favour of code I wrote myself.

Original comment by marc.noi...@gmail.com on 21 Jul 2012 at 1:56

• Changed state: Verified