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
