[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Unsafe WC DB calls: sqlite_column_text(..., pool=NULL)

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Tue, 29 Jan 2013 19:36:10 +0000 (GMT)

Bert Huijben wrote:
> Bert Huijben wrote:
>> Julian Foad wrote:
>>> I noticed code like this in wc_db.c:
>>>   const char *relpath = svn_sqlite__column_text(stmt, 0, NULL);
>>>   svn_kind_t kind = svn_sqlite__column_token(stmt, 1, kind_map);
>>> According to the docs [1], the second _column_*() call can overwrite
>>> the value returned by the first one, since we passed NULL as the
>>> 'result_pool' argument.
>> Then we should fix our docs.

Yes.  After reading our doc string, when I then read the SQLite docs I carried with me the idea that any text retrieval could invalidate a previous one, but the SQLite docs don't in fact say that.

Fixed in r1440071:

 /* Wrapper around sqlite3_column_text. If the column is null, then the
-   return value will be NULL. If RESULT_POOL is not NULL, allocate the
-   return value (if any) in it. Otherwise, the value will become invalid
-   on the next invocation of svn_sqlite__column_* */
+   return value will be NULL.
+   If RESULT_POOL is not NULL, allocate the return value (if any) in it.
+   If RESULT_POOL is NULL, the return value will be valid until an
+   invocation of svn_sqlite__column_* performs a data type conversion (as
+   described in the SQLite documentation) or the statement is stepped or
+   reset or finalized. */
 const char *svn_sqlite__column_text(..., result_pool);


>> It is safe until you go to the next row.
> Looking at the sqlite docs: unless there are type conversions, but in this
> case there aren't any.
> Nor should there be any of these conversions in our code as we don't use
> utf-16
> You might be able to trigger them when accessing wc.db with different tools
> that do insert some utf-16, but I don't think we should update all our code
> to handle that, for some theoretical corner cases.

Why the focus on UTF-16?  Any conversion that SQLite can't do "in place" can invalidate the result, such as blob -> text, blob -> number, or number -> text.

I don't see any reason why we should intentionally have any type conversions in the WC DB code, so I guess we can simply declare "This is safe because we have no type conversions".

- Julian
Received on 2013-01-29 20:36:46 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.