Re: Possible bug in pc+-file-reader.c ??

[Ben Pfaff]

On Sat, Sep 12, 2015 at 09:19:18AM +0200, John Darrington wrote:
> I think the implementation of pcp_detect is buggy.
> Amoungst others, valgrind is complaining about this code:
> 
> 
> 
> /* Returns true if FILE is an SPSS/PC+ system file,
>    false otherwise. */
> static int
> pcp_detect (FILE *file)
> {
>   static const char signature[4] = "SPSS";
>   char buf[sizeof signature];
> 
>   if (fseek (file, 0x104, SEEK_SET)
>       || (fread (buf, sizeof buf, 1, file) != 1 && !feof (file)))
>     return -errno;
> 
>   return !memcmp (buf, signature, sizeof buf);
> }
> 
> 
> There are several issues I see here:
> 
> If the "return -errno" path is taken, then the comment seems to be incorrect.
> Assuming that errno is non-zero the return value is "true" which cannot be 
> correct.
> fread will return 0 meaning that the comparison 0 != 1 will 
> 
> 
> Secondly, the POSIX specification for fseek says this:
> 
>  "The fseek() function allows the file-position indicator to be set beyond 
> the 
>   end of existing data in the file."
> 
> It seems that the code has assumed fseek will fail if the file is 
> less than 0x104 bytes long.  So in my case (I have a short file)
> fseek succeeds, fread fails (returns 0) .  Hence memcmp is getting
> called when buf is uninitialized.
> 
> 
> Is my analysis correct or have I missed something?

Thanks for the bug report.  It's a real bug.  I fixed it.