[BUG] Incorrect use of %d -> db corruption or crash?

[chris0]

I noticed several reports of SQLite database corruption, so I thought I’d have a quick browse through the source (as I’m quite familiar with SQLite).

The incorrect use of the ‘%d’ format specifier in SQL statements (via calls to SQLDatabase-performQueryWithFormat: & Database-executeSQLWithFormat:) could lead to possible db corruption or crashes.

The ‘%d’ format specifier is specified to accept a 32-bit integer.
In multiple locations an NSInteger is being passed.
On x86-64 (& PPC-64) an NSInteger is typedef’ed as ‘long’ — which is a 64-bit integer. [On i386 & PPC NSInteger is 32-bit.]
At the very least this will result in the wrong values being passed to SQL statements & SQLite.

The formatters should be replaced with ‘%ld’ as this is 32-bit on i386 & PPC & 64-bit on x86-64 & PPC-64.
Also, I noticed several occurrences of the ‘%i’ format specifier. While this is probably equivalent to ‘%d’ it is not actually defined for NSString-initWithFormat:arguments: which is the ultimate consumer of the format strings.
See <https://developer.apple.com/library/mac ... fiers.html>.

Regards,

Chris

[barijaona]

Thanks for the observation.

After another quick look at the code, I think it shouldn't be an issue, because data passed are small integers, like folder's id.
But I will patch it anyway. Thanks again for your attention.

[barijaona]

I tried to replace %d with %ld, but some behaviors changed after that... I had to revert it back.
It might be linked to the fact that Vienna is storing booleans by representing them with integers in the database...
Any idea on doing it right ?

[chris0]

> I tried to replace %d with %ld, but some behaviors changed after that... I had to revert it back.
You haven’t stated precisely what you changed or what the results were, but a global search & replace would be incorrect.
For example, in Database.m:

Code: Select all

-(NSInteger)addGoogleReaderFolder:(NSString *)feedName underParent:(NSInteger)parentId afterChild:(NSInteger)predecessorId subscriptionURL:(NSString *)url {
…
		SQLResult * results = [sqlDatabase performQueryWithFormat:
							   @"insert into rss_folders (folder_id, description, username, home_page, last_update_string, feed_url, bloglines_id) "
							   "values (%d, '%@', '', '', '', '%@', %d)",
							   folderId,
							   preparedName,
							   preparedURL,
							   0];

The first %d should be %ld as `folderId` is a long, but the second should remain unchanged as `0` is an int.
[Or you could replace the second %d with a literal 0 in this instance.]

and:

Code: Select all

-(void)markArticleRead:(NSInteger)folderId guid:(NSString *)guid isRead:(BOOL)isRead
…
			SQLResult * results = [sqlDatabase performQueryWithFormat:@"update messages set read_flag=%d where folder_id=%d and message_id='%@'", isRead, folderId, preparedGuid];

The first %d is OK as `isRead` is a BOOL but the second should be %ld as `folderId` is a long.

Every performQueryWithFormat: & executeSQLWithFormat: call needs to be checked individually.

> It might be linked to the fact that Vienna is storing booleans by representing them with integers in the database...
That’s normal & shouldn’t present a problem if you use %d for (Obj-C) BOOL & int values.

BTW Database-executeSQLWithFormat: is missing a call to va_end.

[barijaona]

Thanks a lot. I think I got it correctly now.