[Fix] Defensive infinite loop guard and UTF-8 check in C API - #20348

After thinking some more about this I have doubts about adding the check there without a #ifdef DEBUG guard - maybe we need to be more aggressive with the documentation, but I don't like the performance implication of always performing UTF-8 validity analysis...

do you think the performance implication of calling auto utf_type = duckdb::Utf8Proc::Analyze(str, str_len, &reason, &pos); for each string here is acceptable, or should we expose an additional function? Or maybe only perform the check behind a #ifdef DEBUG guard?

Acceptable and necessary. DuckDB expects any VARCHAR that has entered the system to be valid UTF-8. Allowing users to enter unexpected data into the system can cause unexpected behaviour (e.g., the infinite loop, which could be abused to cause the system to hang: potential denial of service).

If we don't add the check here ourselves, any application developer that uses DuckDB and handles user data will have to add it themselves, as they wouldn't ever want DuckDB to exhibit unexpected behaviour.

Just as an example: strings in Parquet are required to be valid UTF-8 as per the Parquet spec. Despite this, we still run UTF-8 validation upon reading strings from Parquet, because user input cannot be trusted. It's not free, but it's also not prohibitively expensive - out Parquet reader performance is fine.

EDIT: you changes look good to me, but let's discuss

Looking at the source here https://github.com/duckdb/duckdb/blob/main/third_party/utf8proc/utf8proc_wrapper.cpp#L80-L87 it looks like this function has an optimisation to quickly skip over ASCII chars so for ASCII-only strings the impact should be minimal (correct me if I am wrong here - not exactly the sharpest C/C++ programmer 😆 )

I added this a while ago. The optimisation specifically improves performance for longer strings by checking 8 bytes at a time. The performance of verifying UTF-8 strings should be relatively OK, but above all it's about correctness and safety.

nice! I also prefer throwing an exception there :)
BTW I noticed that the same problem probably exists also in translate.cpp and utf8proc_wrapper.cpp

If we don't add the check here ourselves, any application developer that uses DuckDB and handles user data will have to add it themselves, as they wouldn't ever want DuckDB to exhibit unexpected behaviour.

@lnkuiper - I think in this case application developers still have to add an extra check because, as-of now, duckdb_vector_assign_string_element cannot return an error. So while we won't assign invalid UTF-8, we also don't tell the user that something went wrong and eventually pass invalid data chunks into DuckDB.

I propose that in this PR I'll elaborate more in the function comment that when using duckdb_vector_assign_string_element[_len] users must provide valid UTF-8 for VARCHAR. I'll also open a follow-up PR to main exposing duckdb_safe_vector_assign_string_element[_len], which returns error data, if VARCHAR and the string is not valid UTF-8, and mark the unsafe functions as outdated.

Imagine we only added the assertion in the GetResultLength function but still allowed invalid UTF-8 VARCHARs, a similar issue could pop up later in a different string function.

@lorenzowritescode - agreed, we need to both prevent the loop and ensure it is much harder to enter invalid input.

BTW I noticed that the same problem probably exists also in translate.cpp and utf8proc_wrapper.cpp

@EtgarDev - thanks! I'll have a look :)

@taniabogatsch Maybe we can return the error in duckdb_appender_flush then? This one can return an error, and then we could verify the entire Vector in one go

Hmm, we could do that - however, this function can be used in many places, for example also when executing custom UDFs. The original issue was found during scalar UDF execution.

We've had some issues w.r.t. when and where to properly return errors in the C API. @Maxxen has been working on more context-awareness and ways to pass and inspect contexts, which can then contain errors (next to other things). However, we don't have that (yet).

We could also refactor the duckdb_vector into a wrapper, as we do with other C API structures, like the appender. That would have no external effects and we could then set the error in the vector itself. Then, we could expose duckdb_vector_get_error to periodically check the error...

I see. I understand that it's difficult right now to get this to throw, so I am fine with improving our docs, but this really should always throw in the future.

We could consider performing this check after custom UDF execution too. We don't want user strings to be a potential gateway to introducing unexpected behaviour.

Thanks a lot for the input + discussion! Coming back to this after thinking about it a bit further. The C API is kind of different from, e.g., parquet files. It is not directly exposed to end users, but other developers. For example, we hand out raw void * pointers for writing to vectors, and have a lot of "unsafe" functions / behaviors.

That being said, preventing each client + extension from having to implement these valid UTF-8 guards themselves is very convenient and a sensible choice. And I fully agree with us throwing on incorrect input, plus that we should try our best to prevent (accidental) incorrect input through the C API.

Since we cannot throw in C API functions and duckdb_vector_assign_string_element_[len] cannot return any error information, I've removed the check from it now and further elaborated on the documentation. I felt like leaving it as-it makes other errors even harder to detect ("I am sure I called the function to set the string but it did nothing!").

Once we merge this PR + andium into main, I'll add new duckdb_vector_safe_assign_... functions and outdate the current ones. Since v1.4 is a bug fix branch I unfortunately cannot change the C API here. So until we've patched this on main the additional guard in the GetResultLength function and the improved documentation must suffice, I'd say.

This PR is of interest to us! We have seen instances of queries never finishes (symptoms are one or two CPU cores consumed at 100% and remaining that way forever or the query just never returns). We know some of our usage has invalid unicode strings. We evaluated doing a pretest for invalid unicode in Golang but deemed it very expensive. We would be very interested if you all exposed a high-speed unicode validator or a separate API in the C-appender that does unicode validation of its inputs (also for the variant that accepts a full datachunk).

you all exposed a high-speed unicode validator or a separate API in the C-appender that does unicode validation of its inputs (also for the variant that accepts a full datachunk).

The current plan is to expose a new C API function for writing UTF-8-validated strings to data chunks, which should then cover all cases where we create (and later ingest) such data chunks.

Thanks!

Here's the follow-up PR with the new safe C API function: #20467