search

boludoz / lz4

Automatically exported from code.google.com/p/lz4
Other
0 stars 0 forks source link

Static code analysis warnings #157

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Run clang static code analyser on source files

What is the expected output? What do you see instead?
Expected: no warnings
Actual: 2 warnings:

================ WARNING 1 ================

lz4/lz4.c:1154:54: warning: Result of 'calloc' is converted to a pointer of 
type 'LZ4_streamDecode_t', which is incompatible with sizeof operand type 'U64'
    LZ4_streamDecode_t* lz4s = (LZ4_streamDecode_t*) ALLOCATOR(sizeof(U64), LZ4_STREAMDECODESIZE_U64);
    ~~~~~~~~~~~~~~~~~~~                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/stefan/Documents/Projects/Indie/pulse-swift/Pods/lz4/lz4.c:142:24: note: 
expanded from macro 'ALLOCATOR'
#define ALLOCATOR(n,s) calloc(n,s)
                       ^~~~~~
1 warning generated.

================ END WARNING 1 ================

================ WARNING 2 ================

lz4/lz4frame.c:202:19: warning: Array access (from variable 'srcPtr') results 
in a null pointer dereference
    U32 value32 = srcPtr[0];
                  ^~~~~~~~~
1 warning generated.

In LZ4F_decompress, all line numbers in lz4frame.c:
1. [line 962] 'selectedIn' initialised to a null pointer value
2. [line 967] Assuming 'decompressOptionsPtr' is not equal to null
3. [line 979] Entering loop body
4. [line 1278] Passing null pointer value via 1st parameter 'srcPtr'
5. [line 1278] Calling 'LZ4F_readLE32'
6. [line 200] Entered call from 'LZ4F_decompress'
7. [line 202] Array access (from variable 'srcPtr') results in a null pointer 
dereference

================ END WARNING 2 ================

What version of the product are you using? On what operating system?
r127 on iOS

Please provide any additional information below.
The easiest way to reproduce this for iOS is to create a new iOS project in 
Xcode, and build the LZ4 sources in there. I did it using Cocoapods.

Original issue reported on code.google.com by stefan.v...@gmail.com on 11 Mar 2015 at 2:03

GoogleCodeExporter commented 9 years ago 
Thanks for notification Stefan.

Unfortunately, I don't have access to Xcode currently, so can't reproduce the 
test independently. But I'll trust your report.

Warning 1 underlines, maybe by chance, that there is a small, harmless, error 
in :

ALLOCATOR(sizeof(U64), LZ4_STREAMDECODESIZE_U64)

The arguments should be swapped :
ALLOCATOR(LZ4_STREAMDECODESIZE_U64, sizeof(U64))
because the macro prototype is 
ALLOCATOR(n,s) calloc(n,s)

Not sure if it solves the warning, and it doesn't change the amount of reserved 
memory either. It just makes it clearer for calloc() that we want the buffer to 
be 8-bytes aligned.

If it doesn't solve the warning, maybe it will be necessary to stop using 
calloc and use malloc instead.

Warning 2 is more troublesome, because it's a false positive.

The static analyzer cannot know that it's impossible for the function to go 
straight from initialization block to dstage_checkSuffix, where the warning 
lies.
dstage_checkSuffix is always and only accessed after selectedIn gets properly 
initialized,
either at dstage_getSuffix or dstage_storeSuffix.

Maybe I would need to find a way to write the code in a different way, which 
would not confuse the static analyzer. Not exactly a top priority item though, 
but if it can help making the code clearer, it's still welcomed.

Regards

Original comment by yann.col...@gmail.com on 11 Mar 2015 at 2:37

GoogleCodeExporter commented 9 years ago 
@Yann,

Real entity of Xcode's static analyzer is "Clang Static Analyzer" [1].
You can install it to Linux platform easily. But since static analyzer is still 
WIP stage, official "LLVM Debian/Ubuntu nightly packages" [2] may be good 
source for it.
The following commands are typical installation and analyze procedure.

# Install clang package
sudo apt-get install -y clang-3.5
scan-build-3.5 -v

# Static analysis
cd /path/to/lz4/programs/
make clean
scan-build-3.5 make

See "scan-build: running the analyzer from the command line" [3] for 
scan-build's usage.

[1] http://clang-analyzer.llvm.org/
[2] http://llvm.org/apt/
[3] http://clang-analyzer.llvm.org/scan-build.html

Original comment by takayuki...@gmail.com on 11 Mar 2015 at 6:56

GoogleCodeExporter commented 9 years ago 
Thanks Takayuki.
I wasn't aware of scan-build.
Indeed, it's possible to reproduce the test now.

Best Regards :)

Original comment by yann.col...@gmail.com on 11 Mar 2015 at 9:06

GoogleCodeExporter commented 9 years ago

Original comment by yann.col...@gmail.com on 15 Mar 2015 at 12:46

GoogleCodeExporter commented 9 years ago

Original comment by yann.col...@gmail.com on 15 Mar 2015 at 12:46

GoogleCodeExporter commented 9 years ago 
There's a new version in the dev branch
https://github.com/Cyan4973/lz4/tree/dev
which seems to fix this issue.

They were both false positive.
There was no problem, but the static analyzer couldn't guess it.
I had the feeling that I could use this opportunity to make the code a bit more 
explicit.

Which I did. I also extended the use of scan-build to the entire branch.
So hopefully, it doesn't detect any other false positive anymore.

It's a minor improvement, but now it is an automated one.
Hopefully, it should help strengthen reliability of the code for the future.

Original comment by yann.col...@gmail.com on 15 Mar 2015 at 12:49

GoogleCodeExporter commented 9 years ago

Original comment by yann.col...@gmail.com on 16 Mar 2015 at 11:42

GoogleCodeExporter commented 9 years ago 
I can confirm that the dev branch solves this problem. Cannot wait for it to be 
merged back :)

Question: what should I use as the 'official' source code repository for lz4? 
The google code repo, or the github repo?

Original comment by stefan.v...@gmail.com on 16 Mar 2015 at 8:50

GoogleCodeExporter commented 9 years ago 
Starting r128 (next version), Github will become the official repo.

Original comment by yann.col...@gmail.com on 16 Mar 2015 at 8:53