Fix DuckDB strings sometimes having garbage on their tails - #84
Conversation
The duckdb driver used this code to deliver strings to Lualand:
if (duckdb_string_is_inlined(v)) {
lua_pushstring(L, v.value.inlined.inlined);
...
lua_pushstring() does a strlen()-like check for string length. This
turns out is not reliable. The duckdb_string_t is defined like this,
at the time of writing, in DuckDB code:
typedef struct {
union {
struct {
uint32_t length;
char prefix[4];
char *ptr;
} pointer;
struct {
uint32_t length;
char inlined[12]; // <----
} inlined;
} value;
} duckdb_string_t;
There is no guarantee that inlined[12] has a null-terminating byte.
I've changed the code to use lua_pushlstring instead, which explicitly
takes a length. Uses the 'uint32_t length' field there.
I found this bug by noticing unexplained garbage after my strings in
a project.
|
I would maybe add that the only Lua I myself tested this fix with was LuaJIT (a constraint of the project I used this with). I tested on a Fedora 43 and also checked on MacOS. I also did a quick check in DuckDB git history to see if the |
|
I want to show someone this bug as an example (teaching thing) which has motivated me to give some reproduction steps for easy following. I did this on a Gentoo Linux, with Lua 5.4. ⠀ ― Test setup ―Install Command I used to install $ luarocks install --local luadbi-duckdbOutput of `luarocks install --local luadbi-duckdb`⠀ ― Test data ―Create a $ duckdb test.duckdb
DuckDB v1.4.3 (Andium)
Enter ".help" for usage hints.
D CREATE TABLE table_of_sins ( sin_description TEXT );
D INSERT INTO table_of_sins VALUES ( 'pineapples' ); -- 10 chars
D INSERT INTO table_of_sins VALUES ( 'Mr. John Lua' ); -- 12 chars (the bug triggers for this one)
D INSERT INTO table_of_sins VALUES ( 'hello world' ); -- 11 chars
D(CTRL+D at the end to exit ⠀ Should now have: $ ls -lah
total 532K
drwxr-xr-x 2 shannon shannon 4.0K Jan 23 22:44 .
drwxr-xr-x 7 shannon shannon 4.0K Jan 23 22:44 ..
-rw-r--r-- 1 shannon shannon 524K Jan 23 22:43 test.duckdb # (...TIL a fresh empty duckdb is ~500kb)⠀ ― Test script ―Hack together -- test.lua
require('luarocks.loader') -- for search paths
local DBI = require('DBI')
-- ignoring all errors because this is a demo
local db, err = DBI.Connect('DuckDB', 'test.duckdb')
local statement, err = db:prepare('SELECT sin_description FROM table_of_sins')
local status, err = statement:execute()
local get_row, err = statement: rows()
local row = get_row()
while row ~= nil do
print(string.format('This string has length %d and prints out as: %q', #row[1], row[1]))
row = get_row()
end⠀ ― Bug ―Run it: $ lua5.4 test.lua
This string has length 10 and prints out as: "pineapples"
This string has length 13 and prints out as: "Mr. John Lua9" # <-- extra 9 has appeared here
This string has length 11 and prints out as: "hello world"It didn't show up for me in my project work all the time, but this one showed up on first try when I was writing these steps. I never saw very long texts after, I think usually 2 characters. |
|
Thank you for this pull request, for contributing in general. As the original author of this driver I'm glad someone found it useful and/or promising enough to create a pull request! I strongly prefer to manually review and verify pull requests before merging them, and I simply haven't had time yet. I see this here, though, and intend to do that review - please be patient. |
2 participants
I was using this project to interface with DuckDB, and I noticed unexplained garbage occasionally on my results, which I traced here.
The DuckDB string structure is this union structure, and the char[12] inlined variant is not guaranteed to be null-terminated, it would seem.
I changed the
lua_pushstringtolua_pushlstringso it can use the explicit length provided.For testing...well if I'm honest I could not figure out how to successfully run the test suite in the project itself, so I tested with my own project that suffered from the bug, and it seems to have fixed it. I've run the codepath that does not use
named_columns, but the fix is quite simple.Reference to the string type in duckdb: https://github.com/duckdb/duckdb/blob/9612b5bea5a6df924daf5ce696d6992df2483bfe/src/include/duckdb.h#L421-L433
I've never really looked into duckdb C API before, and I'm a bit surprised that structure is being accessed directly anyway; I did see there is
duckdb_string_t_lengthandduckdb_string_t_datathat seem more like a public API. I don't know the original motivations of this code why it does not use the public-looking API (maybe performance of avoiding a function call?), but since I don't have time to do tests and performance measuring stuff (I want to go back to my project :-)), I left this as a short targeted fix.