Re: [sqlite] Bug report: USBAN failure

[Scott Robison]

On Wed, Dec 3, 2014 at 5:46 PM, Simon Slavin  wrote:

>
> On 4 Dec 2014, at 12:26am, James K. Lowden 
> wrote:
>
> > What to do is another question.  SQLite can surely ignore it.  If I
> > felt strongly about it, I'd submit a bug report to GCC because IIUC
> > the nonnull attribute syntax provides no way to express the constraint
> > defined by the standard, i.e. "__n > 0 || __nonnull ((1, 2))".
>
> But that's not what the standard says.  My understanding of the standard
> is that both source and destination must be legitimate memory locations.
> It says nothing about the value of n.  In other words memcpy() may well
> look at the source and destination locations before discovering that n == 0
> so it doesn't have to do anything. [1]  It's clear even in the first
> standard Clemens quoted.
>

That is the C99 standard. C90 did not have that restriction. A lot of
projects target C90 for maximum portability. In this particular case it
does not make SQLite incompatible with C90 to accommodate C99, so it has
been accommodated. Still, I would maintain that the standard being used was
not being violated in this case.

-- 
Scott Robison
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[James K. Lowden]

On Wed, 03 Dec 2014 08:56:44 +0100
Clemens Ladisch  wrote:

> James K. Lowden wrote:
> > /* Copy N bytes of SRC to DEST.  */
> > extern void *memcpy (void *__restrict __dest,
> >  __const void *__restrict __src, size_t __n)
> >  __THROW __nonnull ((1, 2));
> >
> > IIUC the declaration specifies the pointer cannot be NULL and the
> > compiler generates a diagnostic if it notices that it can be.  But
> > the declaration is strictly stricter than the standard defines.
> 
> Do you have a standard that allows NULL?  The one I quoted does not.

I'm sure you're right about the standard.  I merely observed that my
not-too-recent copy of strings.h includes a nonnull attribute.
Strictly speaking, that attribute may be "wrong" in the sense that the
standard does permit the pointer to be NULL if the length is zero.  A
compiler may nevertheless emit a diagnostic when it comes across one.
For gcc that's triggered with -Wnonnull.  I think that might explain the
message the OP saw.  

What to do is another question.  SQLite can surely ignore it.  If I
felt strongly about it, I'd submit a bug report to GCC because IIUC
the nonnull attribute syntax provides no way to express the constraint
defined by the standard, i.e. "__n > 0 || __nonnull ((1, 2))".  

--jkl
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Scott Robison]

On Wed, Dec 3, 2014 at 9:50 AM, Stephan Beal  wrote:

> On Wed, Dec 3, 2014 at 5:35 PM, Scott Robison 
> wrote:
>
> > standards have all been ISO standards. Pedantic? Yes. Obviously DRH is
> > willing to make the code more portable as long as it doesn't violate
> > ANSI-C, hence his patch early in the thread (see
> > https://www.sqlite.org/src/info/0d04f380e1bd17104b3cf76b64d0cfc79a726606
> ).
> >
>
> Just to harmlessly nitpick: sqlite (necessarily) uses long long for int64,
> which isn't strictly C89: but works on essentially every compiler.
>

Fair enough, though I think the selection of long long is wrapped up in
conditionals, isn't it? Conditional blocks are there for this purpose! :)

{checks} Yes:

#ifdef SQLITE_INT64_TYPE

  typedef SQLITE_INT64_TYPE sqlite_int64;

  typedef unsigned SQLITE_INT64_TYPE sqlite_uint64;

#elif defined(_MSC_VER) || defined(__BORLANDC__)

  typedef __int64 sqlite_int64;

  typedef unsigned __int64 sqlite_uint64;

#else

  typedef long long int sqlite_int64;

  typedef unsigned long long int sqlite_uint64;

#endif

typedef sqlite_int64 sqlite3_int64;

typedef sqlite_uint64 sqlite3_uint64;


It only falls to long long if nothing better has been provided or found.

-- 
Scott Robison
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Stephan Beal]

On Wed, Dec 3, 2014 at 5:35 PM, Scott Robison 
wrote:

> standards have all been ISO standards. Pedantic? Yes. Obviously DRH is
> willing to make the code more portable as long as it doesn't violate
> ANSI-C, hence his patch early in the thread (see
> https://www.sqlite.org/src/info/0d04f380e1bd17104b3cf76b64d0cfc79a726606).
>

Just to harmlessly nitpick: sqlite (necessarily) uses long long for int64,
which isn't strictly C89: but works on essentially every compiler.

gcc -c -pedantic -std=c89 -Wall -Werror sqlite3.c
sqlite3.c:377:16: error: ISO C90 does not support ‘long long’
[-Werror=long-long]
   typedef long long int sqlite_int64;
^
sqlite3.c:378:25: error: ISO C90 does not support ‘long long’
[-Werror=long-long]
   typedef unsigned long long int sqlite_uint64;
 ^
cc1: all warnings being treated as errors


clang -c -pedantic -std=c89 -Wall -Werror sqlite3.c
sqlite3.c:377:11: error: 'long long' is an extension when C99 mode is not
enabled [-Werror,-Wlong-long]
  typedef long long int sqlite_int64;
  ^
sqlite3.c:378:20: error: 'long long' is an extension when C99 mode is not
enabled [-Werror,-Wlong-long]
  typedef unsigned long long int sqlite_uint64;


which can be squelched with:

gcc|clang -c -pedantic -std=c89 -Wall -Werror -Wno-long-long sqlite3.c

(clang now reports an unused var, but that's something else.)


-- 
- stephan beal
http://wanderinghorse.net/home/stephan/
http://gplus.to/sgbeal
"Freedom is sloppy. But since tyranny's the only guaranteed byproduct of
those who insist on a perfect world, freedom will have to do." -- Bigby Wolf
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Scott Robison]

On Wed, Dec 3, 2014 at 2:17 AM, Clemens Ladisch  wrote:

> Scott Robison wrote:
> > On Dec 3, 2014 12:57 AM, "Clemens Ladisch"  wrote:
> >> Do you have a standard that allows NULL?  The one I quoted does not.
> >
> > Note: I'll have to double check my copy of the C90 standard document, but
> > my re-reading of the C99 quote leads me to the conclusion that NULL is a
> > valid pointer by definition.
>
> Then we have to go reference chasing:
>
> C99, 7.21.1:
> | Unless explicitly stated otherwise in the description of a particular
> | function in this subclause, pointer arguments on such a call shall
> | still have valid values, as described in 7.1.4.
>
> 7.1.4:
> | Each of the following statements applies unless explicitly stated
> | otherwise in the detailed descriptions that follow: If an argument to
> | a function has an invalid value (such as ... a null pointer ...) ...,
> | the behavior is undefined.
>

Okay, fair enough. I'm comparing the C90 & C90 (2005) standards at the
moment. The equivalent clause in C90 7.11, but in C90, it omits the second
paragraph (from which the quote was extracted). So C90 did not prohibit
NULL values beyond their implicit undefined behavior on dereference as per
6.3.3.2 (Address and indirection operators: "... If an invalid value has
been assigned to the pointer, the behavior of the unary * operator is
undefined.[42]" and ("[42] ... Among the invalid values for dereferencing a
pointer by the unary * operator are a null pointer ...").

I can certainly appreciate why the committee opted to make the change in
C99, though in this case it seems that (at least) memcpy & memset *should*
(but don't) have exceptions for those functions, since there is never a
need to dereference a pointer if the size of the block is zero. Other
"real" string functions (like strncpy) have the extra requirement of
finding a null terminator, so I can understand why a NULL source pointer
would be undefined behavior there, but not memcpy.

In any case, "SQLite is ANSI-C source code" (as per
http://www.sqlite.org/howtocompile.html). The only C standard that was
developed by ANSI was C89. C90 (almost identical to C89) and subsequent
standards have all been ISO standards. Pedantic? Yes. Obviously DRH is
willing to make the code more portable as long as it doesn't violate
ANSI-C, hence his patch early in the thread (see
https://www.sqlite.org/src/info/0d04f380e1bd17104b3cf76b64d0cfc79a726606).

Perhaps the R language project just needs to pick the "correct" dialect for
GCC (-std=C89 seems appropriate) when compiling the amalgamation? Might not
help if the UBSan markup in the header doesn't change based on the dialect
in use, but it might permit SQLite to compile cleanly. Unless it's already
being done, in which case I guess not. And it's moot, since it has been
accommodated in the source.

-- 
Scott Robison
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Clemens Ladisch]

Scott Robison wrote:
> On Dec 3, 2014 12:57 AM, "Clemens Ladisch"  wrote:
>> Do you have a standard that allows NULL?  The one I quoted does not.
>
> Note: I'll have to double check my copy of the C90 standard document, but
> my re-reading of the C99 quote leads me to the conclusion that NULL is a
> valid pointer by definition.

Then we have to go reference chasing:

C99, 7.21.1:
| Unless explicitly stated otherwise in the description of a particular
| function in this subclause, pointer arguments on such a call shall
| still have valid values, as described in 7.1.4.

7.1.4:
| Each of the following statements applies unless explicitly stated
| otherwise in the detailed descriptions that follow: If an argument to
| a function has an invalid value (such as ... a null pointer ...) ...,
| the behavior is undefined.


Regards,
Clemens
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Scott Robison]

On Dec 3, 2014 12:57 AM, "Clemens Ladisch"  wrote:
>
> Do you have a standard that allows NULL?  The one I quoted does not.

Note: I'll have to double check my copy of the C90 standard document, but
my re-reading of the C99 quote leads me to the conclusion that NULL is a
valid pointer by definition. Dereference of NULL is undefined, but as a
value for a pointer it is defined.
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Scott Robison]

On Dec 3, 2014 12:57 AM, "Clemens Ladisch"  wrote:
>
> James K. Lowden wrote:
> > /* Copy N bytes of SRC to DEST.  */
> > extern void *memcpy (void *__restrict __dest,
> >  __const void *__restrict __src, size_t __n)
> >  __THROW __nonnull ((1, 2));
> >
> > IIUC the declaration specifies the pointer cannot be NULL and the
> > compiler generates a diagnostic if it notices that it can be.  But the
> > declaration is strictly stricter than the standard defines.
>
> Do you have a standard that allows NULL?  The one I quoted does not.

*The* standard is completely silent on passing NULL to memcpy or memset
(other than dereference of NULL is undefined behavior). With a size of 0
there will never be a dereference. The library in use is trying to warn you
about potential errors but in this case isn't smart enough to detect the 0
size value.

I can appreciate why the "extra" cautious warning exists given how often
error checking is ignored, but since the ubsan infrastructure works by
instrumenting the code, an assert would arguably be a better approach that
would be smart enough to detect more than the non null attribute is capable
of. That doesn't help the current mistaken impression that there was a
problem with SQLite that didn't really exist. Which is moot since DRH
already worked around the "broken" warning. :)
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Clemens Ladisch]

James K. Lowden wrote:
> /* Copy N bytes of SRC to DEST.  */
> extern void *memcpy (void *__restrict __dest,
>  __const void *__restrict __src, size_t __n)
>  __THROW __nonnull ((1, 2));
>
> IIUC the declaration specifies the pointer cannot be NULL and the
> compiler generates a diagnostic if it notices that it can be.  But the
> declaration is strictly stricter than the standard defines.

Do you have a standard that allows NULL?  The one I quoted does not.


Regards,
Clemens
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[James K. Lowden]

On Tue, 02 Dec 2014 15:58:47 +0100
Abramo Bagnara  wrote:

> The point is not about overzealousness, but about the declaration of
> memcpy/memset on your machine.
> 
> If it contains the nonnull attribute then (correctly) UBSan detect
> that such constraint is not respected.

Hmm, I guess you mean this (from my handy LTS Ubuntu box):  

/* Copy N bytes of SRC to DEST.  */
extern void *memcpy (void *__restrict __dest,
 __const void *__restrict __src, size_t __n)
 __THROW __nonnull ((1, 2));

(Documentation at 
https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bnonnull_007d-function-attribute-2263.)

IIUC the declaration specifies the pointer cannot be NULL and the
compiler generates a diagnostic if it notices that it can be.  But the
declaration is strictly stricter than the standard defines.  

--jkl
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Hadley Wickham]

>> The block of code that refers to is:
>>
>>   if( p->azVar ){
>> p->nzVar = pParse->nzVar;
>> memcpy(p->azVar, pParse->azVar, p->nzVar*sizeof(p->azVar[0]));
>> memset(pParse->azVar, 0, pParse->nzVar*sizeof(pParse->azVar[0]));
>>   }
>>
>> So maybe the check should be on (pParse->azVar) ?
>>
>
> No, that would result in NULL pointer dereferences following an
> out-of-memory condition.  The correct work-around (it isn't really a bug
> fix) is to test pParse->nzVar in addition to p->azVar.  See
> https://www.sqlite.org/src/info/0d04f380e1bd17104b3cf76b64d0cfc79a726606
> for the change.

Thanks, it's much appreciated!  (And sorry about the USBAN typo)

Hadley

-- 
http://had.co.nz/
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Abramo Bagnara]

Il 02/12/2014 15:02, Richard Hipp ha scritto:
> On Tue, Dec 2, 2014 at 8:53 AM, Dominique Devienne 
> wrote:
> 
>> On Tue, Dec 2, 2014 at 2:47 PM, Richard Hipp  wrote:
>>
>>> On Mon, Dec 1, 2014 at 5:46 PM, Hadley Wickham 
>>> wrote:
 [...] has started running all R packages with USBAN. This reveals a
>>> problem in sqlite.c
>>>
>>> I'm not sure what USBAN is
>>>
>>
>> Most likely a typo. UBSan, Undefined Behavior Sanitizer (from GCC / Clang).
>>
> 
> OK, that makes sense.
> 
> But we've been running 100% branch coverages tests of SQLite using
> -fsanitize=undefined for a long while now (using clang), and we've never
> encountered the issue that Hadley is having.  So some other kind of
> overzealousness is going on here as well, it seems.

The point is not about overzealousness, but about the declaration of
memcpy/memset on your machine.

If it contains the nonnull attribute then (correctly) UBSan detect that
such constraint is not respected.

-- 
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagn...@bugseng.com
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Richard Hipp]

On Tue, Dec 2, 2014 at 8:53 AM, Dominique Devienne 
wrote:

> On Tue, Dec 2, 2014 at 2:47 PM, Richard Hipp  wrote:
>
> > On Mon, Dec 1, 2014 at 5:46 PM, Hadley Wickham 
> > wrote:
> > > [...] has started running all R packages with USBAN. This reveals a
> > problem in sqlite.c
> >
> > I'm not sure what USBAN is
> >
>
> Most likely a typo. UBSan, Undefined Behavior Sanitizer (from GCC / Clang).
>

OK, that makes sense.

But we've been running 100% branch coverages tests of SQLite using
-fsanitize=undefined for a long while now (using clang), and we've never
encountered the issue that Hadley is having.  So some other kind of
overzealousness is going on here as well, it seems.

-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Clemens Ladisch]

Richard Hipp wrote:
> But apparently there is an issue in USBAN in that it does not allow calls
> to memcpy() and memset() with NULL pointers even it the count field (the
> third parameter) is zero.  I couldn't find anything in the memcpy() or
> memset() documentation that disallowed this case.

C99, section 7.21.1 (String function conventions):
| Where an argument declared as size_t n specifies the length of the
| array for a function, n can have the value zero on a call to that
| function. Unless explicitly stated otherwise in the description of
| a particular function in this subclause, pointer arguments on such
| a call shall still have valid values


Regards,
Clemens
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Dominique Devienne]

On Tue, Dec 2, 2014 at 2:47 PM, Richard Hipp  wrote:

> On Mon, Dec 1, 2014 at 5:46 PM, Hadley Wickham 
> wrote:
> > [...] has started running all R packages with USBAN. This reveals a
> problem in sqlite.c
>
> I'm not sure what USBAN is
>

Most likely a typo. UBSan, Undefined Behavior Sanitizer (from GCC / Clang).
--DD
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Bug report: USBAN failure

[Richard Hipp]

On Mon, Dec 1, 2014 at 5:46 PM, Hadley Wickham  wrote:

> Hi,
>
> I'm the maintainer of RSQLite, the R language binding for SQLite.
> Recently, CRAN (the common R archive network) has started running all
> R packages with USBAN. This reveals a problem in sqlite.c
> (
> http://www.stats.ox.ac.uk/pub/bdr/memtests/UBSAN-gcc/RSQLite/tests/testthat.Rout
> )
>
> > library(RSQLite)
> > con <- dbConnect(SQLite(), dbname = tempfile())
> sqlite/sqlite3.c:63931:5: runtime error: null pointer passed as
> argument 2, which is declared to never be null
> sqlite/sqlite3.c:63932:5: runtime error: null pointer passed as
> argument 1, which is declared to never be null
>

I'm not sure what USBAN is, and a quick search using Firefox (which as of
this morning takes me to Yahoo instead of Google) doesn't provide any clues.

But apparently there is an issue in USBAN in that it does not allow calls
to memcpy() and memset() with NULL pointers even it the count field (the
third parameter) is zero.  I couldn't find anything in the memcpy() or
memset() documentation that disallowed this case.  And SQLite has been
doing that for many years on a huge variety of systems without any
problems.  So I think perhaps USBAN (whatever it is) is being a little
overzealous in its constraints.


>
> The block of code that refers to is:
>
>   if( p->azVar ){
> p->nzVar = pParse->nzVar;
> memcpy(p->azVar, pParse->azVar, p->nzVar*sizeof(p->azVar[0]));
> memset(pParse->azVar, 0, pParse->nzVar*sizeof(pParse->azVar[0]));
>   }
>
> So maybe the check should be on (pParse->azVar) ?
>

No, that would result in NULL pointer dereferences following an
out-of-memory condition.  The correct work-around (it isn't really a bug
fix) is to test pParse->nzVar in addition to p->azVar.  See
https://www.sqlite.org/src/info/0d04f380e1bd17104b3cf76b64d0cfc79a726606
for the change.


-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users

[sqlite] Bug report: USBAN failure

[Hadley Wickham]

Hi,

I'm the maintainer of RSQLite, the R language binding for SQLite.
Recently, CRAN (the common R archive network) has started running all
R packages with USBAN. This reveals a problem in sqlite.c
(http://www.stats.ox.ac.uk/pub/bdr/memtests/UBSAN-gcc/RSQLite/tests/testthat.Rout)

> library(RSQLite)
> con <- dbConnect(SQLite(), dbname = tempfile())
sqlite/sqlite3.c:63931:5: runtime error: null pointer passed as
argument 2, which is declared to never be null
sqlite/sqlite3.c:63932:5: runtime error: null pointer passed as
argument 1, which is declared to never be null

The block of code that refers to is:

  if( p->azVar ){
p->nzVar = pParse->nzVar;
memcpy(p->azVar, pParse->azVar, p->nzVar*sizeof(p->azVar[0]));
memset(pParse->azVar, 0, pParse->nzVar*sizeof(pParse->azVar[0]));
  }

So maybe the check should be on (pParse->azVar) ?

(as of version 3.8.6)

Hadley


-- 
http://had.co.nz/
___
sqlite-users mailing list
sqlite-users@sqlite.org
http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users