2012.07.07 08:38 "Re: [Tiff] Challenge to properly eliminate warning", by Even Rouault
I think that the compiler does understand the complex logic and that there must be cases where no error is returned but the data was not updated. It requires -O2 optimization level before these issues are detected. At this level, GCC is inlining the short functions in the module so it sees a lot of issues it would not otherwise see.
It only requires a single case where the error indication is not set or used correctly so that 'data' gets consumed when it should not be in order to produce this warning.
Unfortunately, there are a multitude of paths executed under TIFFReadDirEntryByte() so it is difficult to determine which ones might include the weakness.
I agree that gcc -O2 is quite clever, but I'm almost sure that in that case it just get lost because of multiple depth of calls, and at some point the functions are no more inlined.
Here's my analysis.
If you comment the 'case TIFF_LONG8' and 'case TIFF_SLONG8' blocks in TIFFReadDirEntryByte(), no more warning. So for now, let's just comment the case TIFF_SLONG8 block:
case TIFF_LONG8:
{
uint64
m;
err=TIFFReadDirEntryCheckedLong8(tif,direntry,&m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
err=TIFFReadDirEntryCheckRangeByteLong8(m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
*value=(uint8)m;
return(TIFFReadDirEntryErrOk);
}
/*case TIFF_SLONG8:
{
int64
m;
err=TIFFReadDirEntryCheckedSlong8(tif,direntry,&m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
err=TIFFReadDirEntryCheckRangeByteSlong8(m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
*value=(uint8)m;
return(TIFFReadDirEntryErrOk);
}
*/
We still have the warning.
If we try this now:
case TIFF_LONG8:
{
uint64
m;
/*err=TIFFReadDirEntryCheckedLong8(tif,direntry,&m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);*/
m
= 0; /* added to avoid warning about m being
uninitialized */
err=TIFFReadDirEntryCheckRangeByteLong8(m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
*value=(uint8)m;
return(TIFFReadDirEntryErrOk);
}
No more warning. Interesting, but there's nothing wrong with the commented code because if TIFFReadDirEntryCheckedLong8 returns something different than TIFFReadDirEntryErrOk, then *value is indeed not initialized, but the callers of that code will not use *value because of the error return code, so that's fine.
So my next hypothesis was that there was perhaps something wrong in TIFFReadDirEntryCheckedLong8() that didn't always initialize m, causing perhaps gcc to warn about *value. Ok, let's test that:
case TIFF_LONG8:
{
uint64
m;
err=TIFFReadDirEntryCheckedLong8(tif,direntry,&m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
m
= 0; /* still there */
err=TIFFReadDirEntryCheckRangeByteLong8(m);
if
(err!=TIFFReadDirEntryErrOk)
return(err);
*value=(uint8)m;
return(TIFFReadDirEntryErrOk);
}
Still the warning about data! This does not make sense.
So I think that there is too much code in that block, directly or indirectly, and that GCC cannot follow all the code paths. We could expect it to not emit a warning in that case, instead of emitting a false positive.
Proposal: at the beginning of TIFFReadDirEntryByte(), add:
Index: tif_dirread.c
===================================================================
--- tif_dirread.c (révision 24659)
+++ tif_dirread.c (copie de travail)
@@ -197,6 +197,7 @@
static enum TIFFReadDirEntryErr TIFFReadDirEntryByte(TIFF* tif, TIFFDirEntry*
direntry, uint8* value)
{
enum TIFFReadDirEntryErr err;
+ *value = 0;
if (direntry->tdir_count!=1)
return