Issue 38 in flvmeta: cppcheck error found and gcc warnings
flv...@googlecode.com
Owner: ----
Labels: Type-Defect Priority-Medium
New issue 38 by neo.neut...@gmail.com: cppcheck error found and gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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
Attachments:
json.diff 314 bytes
suppress-gcc-warnings.diff 1018 bytes
flv...@googlecode.com
Status: Started
Owner: marc.noi...@gmail.com
Comment #1 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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
flv...@googlecode.com
Labels: Milestone-1.1
Comment #2 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
(No comment was entered for this change.)
flv...@googlecode.com
Comment #3 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
Seems like the cppcheck error has been solved upstream:
http://mjson.svn.sourceforge.net/viewvc/mjson/trunk/src/json.c?r1=158&r2=159&pathrev=159
flv...@googlecode.com
Comment #4 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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 :).
flv...@googlecode.com
Comment #5 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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 ? :)
flv...@googlecode.com
Status: Fixed
Comment #6 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
All patches have been applied (sometimes with modifications from your
original patch).
flv...@googlecode.com
Comment #7 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
:)
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 :)
flv...@googlecode.com
Comment #8 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
I have prepared the configure.ac and src/Makefile.am patch
(use-system-libs.diff)
which add the --with-system-libs option that force the builder to use
system libraries.
please review.
Attachments:
use-system-libs.diff 1.2 KB
flv...@googlecode.com
Comment #9 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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.
Attachments:
update-json-compiler-warning.diff 392 bytes
flv...@googlecode.com
Comment #10 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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
Attachments:
update-json-compiler-warning-prix64.diff 503 bytes
flv...@googlecode.com
Comment #11 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
Update:
Paul Wise advices me to do the better patch by implement the PRIX64 instead
flv...@googlecode.com
Status: Started
Labels: -Priority-Medium Priority-High
Comment #12 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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.
flv...@googlecode.com
Comment #13 on issue 38 by neo.neut...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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.
flv...@googlecode.com
Comment #14 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
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.
flv...@googlecode.com
Status: Verified
Comment #15 on issue 38 by marc.noi...@gmail.com: cppcheck error found and
gcc warnings
http://code.google.com/p/flvmeta/issues/detail?id=38
This issue is irrelevant as of 1.1.0, as I have ditched mjson altogether in
favour of code I wrote myself.