svn commit: r285644 - head/contrib/sqlite3

[Bruce Evans]

On Thu, 16 Jul 2015, Pedro Giffuni wrote:

> On 07/16/15 17:22, Ian Lepore wrote:
>> On Thu, 2015-07-16 at 22:07 +0000, Pedro F. Giffuni wrote:
>>> Author: pfg
>>> Date: Thu Jul 16 22:07:13 2015
>>> New Revision: 285644
>>> URL: https://svnweb.freebsd.org/changeset/base/285644
>>> 
>>> Log:
>>>    sqlite: clean a couple of invocations of memcpy(3)
>>>       Found almost accidentally by our native gcc when enhanced with
>>>    FORTIFY_SOURCE.
>>>       Submitted by:	Oliver Pinter
>>>    Sponosored by:	Google Inc. GSoC 2015
>>> 
>>> Modified:
>>>    head/contrib/sqlite3/sqlite3.c
>>> 
>>> Modified: head/contrib/sqlite3/sqlite3.c
>>> ==============================================================================
>>> --- head/contrib/sqlite3/sqlite3.c	Thu Jul 16 19:40:18 2015 
>>> (r285643)
>>> +++ head/contrib/sqlite3/sqlite3.c	Thu Jul 16 22:07:13 2015 
>>> (r285644)
>>> @@ -49487,9 +49487,9 @@ static void walIndexWriteHdr(Wal *pWal){
>>>     pWal->hdr.isInit = 1;
>>>     pWal->hdr.iVersion = WALINDEX_MAX_VERSION;
>>>     walChecksumBytes(1, (u8*)&pWal->hdr, nCksum, 0, pWal->hdr.aCksum);
>>> -  memcpy((void *)&aHdr[1], (void *)&pWal->hdr, sizeof(WalIndexHdr));
>>> +  memcpy((void *)&aHdr[1], (const void *)&pWal->hdr, 
>>> sizeof(WalIndexHdr));
>>>     walShmBarrier(pWal);
>>> -  memcpy((void *)&aHdr[0], (void *)&pWal->hdr, sizeof(WalIndexHdr));
>>> +  memcpy((void *)&aHdr[0], (const void *)&pWal->hdr, 
>>> sizeof(WalIndexHdr));
>>>   }
>>>     /*
>>> 
>> Setting aside any "unnecessary divergence with upstream" questions for
>> the moment, wouldn't the correct fix be to just remove the casting
>> completely?

Indeed.  (Remove all the bogus casts, not just the changed ones.)

> The compiler should know, but it is not wrong to have the casts
> match the (standard) implementation. I couldn't say which is
> "more" correct.

It is not even wrong.  The casts to void * are only needed to support
K&R compilers (ones that aren't actually K&R 1, so that they support
void *), or if the program is so buggy as to not make a prototype for
memcpy() visible.  I think the program doesn't attempt to support K&R.
I checked that it includes string.h.

The hdr variable is not const, so casting it to void * is not even
wrong.  If it were const, then the cast to void * still wouldn't be
wrong (it would still conform to C, and the prototype would undo it).
However, it would remove the const type qualifier, and some compilers
warn about this, so it would be an error at a higher level.

If the code supported K&R, then your change would be correct for the
following reasons:
- K&R needs the void * part of the casts
- K&R doesn't support const (except it is further from actually being
   K&R 1 so that it supports both const and void), so const void * looks
   like a syntax error.  However, the support for K&R in <sys/cdefs.h>
   kills this const so that the K&R case works.
- the full cast should be used.  Otherwise the cast looks more bogus
   since it is only half of not even wrong for the non-K&R case.

Modern programs have many dependencies on prototypes actually working.
E.g., sqlite.c has a measly 257 calls to memcpy().  It bogusly casts
to void * in a whole 8 of these.  Often the args are already void *,
so they don't need need a bogus (null) cast to make them that, but the
second arg does need a bogus (non-null) cast to const void * to make it
that before the prototype does.

Casts that have no effect shouldn't be used since they bloat the source
code and obfuscate the casts that are actually needed.  Exception: it is
good to use explicit downcasts and not depend on the design error that
prototypes downcast without warning.

Bruce