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
Received on 2013-01-29 20:36:46 CET