Static analysis warning

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Static analysis warning

Aaron Boxer
Got this from Visual Studio 2015 static analyzer:

tif_fax3.c(713): warning C6385: Reading invalid data from '_msbmask':  the readable size is '36' bytes, but '52' bytes may be read.


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Static analysis warning

Even Rouault-2

On mercredi 26 avril 2017 16:31:35 CEST Aaron Boxer wrote:

> Got this from Visual Studio 2015 static analyzer:

>

> tif_fax3.c(713): warning C6385: Reading invalid data from '_msbmask': the

> readable size is '36' bytes, but '52' bytes may be read.

 

static const int _msbmask[9] =

{ 0x00, 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 0xff };

 

So the maximum readable size 36 = 9 * 4 is correct.

 

Now I'm wondering why it thinks that 52 = 13 * 4 can be read. Does it provide more details ?

 

The only place where _msbmask is accessed is protected by an assert()

 

assert( length < 9 ); \

data |= (bits & _msbmask[length]) << (bit - length); \

 

Admidetly for my non FAX3 specialist highs, there's nothing obvious that in release mode, where the assert() is a no-op, that length is guaranteed to be < 9, but perhaps there are other guarantees that it is correct from the calling code... So unless you have a test case that demonstrates a real bug, I'd tend to think that this code is correct and this is a false positive.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|

Re: Static analysis warning

Aaron Boxer


On Thu, Apr 27, 2017 at 2:05 PM, Even Rouault <[hidden email]> wrote:

On mercredi 26 avril 2017 16:31:35 CEST Aaron Boxer wrote:

> Got this from Visual Studio 2015 static analyzer:

>

> tif_fax3.c(713): warning C6385: Reading invalid data from '_msbmask': the

> readable size is '36' bytes, but '52' bytes may be read.

 

static const int _msbmask[9] =

{ 0x00, 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 0xff };

 

So the maximum readable size 36 = 9 * 4 is correct.

 

Now I'm wondering why it thinks that 52 = 13 * 4 can be read. Does it provide more details ?

 

The only place where _msbmask is accessed is protected by an assert()

 

assert( length < 9 ); \

data |= (bits & _msbmask[length]) << (bit - length); \

 

Admidetly for my non FAX3 specialist highs, there's nothing obvious that in release mode, where the assert() is a no-op, that length is guaranteed to be < 9, but perhaps there are other guarantees that it is correct from the calling code... So unless you have a test case that demonstrates a real bug, I'd tend to think that this code is correct and this is a false positive.


Yes, a false positive.  And, in fact, when I run static analysis in VS 2017, this issue doesn't appear.

Sorry for the noise,
Aaron




_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/