Re: [sqlite] Bug report: USBAN failure
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
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. In this it appears I agree with Clemens' interpretation, and with the notation in the function definitions: source and dest . I don't know what this says about SQLite's code. Is it the custom these days to include clear and visible null checks in C code even if the programmer(s) know they can never be triggered ? Simon. [1] A naïve view of optimization is that memcpy() should check for (n == 0) first so it can return quickly, but this check slows down (n != 0) use of the function and that makes me ask why your code keeps calling memcpy() for zero bytes often enough for the check to give a significant advantage. ___ sqlite-users mailing list sqlite-users@sqlite.org http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users
Re: [sqlite] Bug report: USBAN failure
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
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
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
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
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
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
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
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
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
>> 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
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
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
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
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
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