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

[Julian Foad <ju...@btopenworld.com>]

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.


Since we haven't run into major breakage, I guess the implementations of SQLite we've been using aren't actually overwriting the data in the situations that we've been testing... but unless we learn that they definitely won't, we should fix this potential error.


If no objections, I'll fix this by passing a pool (scratch_pool, iterpool, whatever has sufficient lifetime) instead of NULL, in all places where it is unsafe.

- Julian


[1] <http://www.sqlite.org/c3ref/column_blob.html>


--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download

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

[Julian Foad <ju...@btopenworld.com>]

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);

etc.

>> 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

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

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: dinsdag 29 januari 2013 18:35
> To: 'Julian Foad'; 'Subversion Development'
> Subject: RE: Unsafe WC DB calls: sqlite_column_text(..., pool=NULL)
> 
> 
> 
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > Sent: dinsdag 29 januari 2013 17:15
> > To: Subversion Development
> > Subject: Unsafe WC DB calls: sqlite_column_text(..., pool=NULL)
> >
> > 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.
> 
> 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.



	Bert 

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

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: dinsdag 29 januari 2013 17:15
> To: Subversion Development
> Subject: Unsafe WC DB calls: sqlite_column_text(..., pool=NULL)
> 
> 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.

It is safe until you go to the next row.

	Bert