Fix/446 diagnostic text utf8 - #447
Merged
Merged
Conversation
PRINT and RAISERROR text reached JavaScript through odbcstr::swcvec2str, which reinterpreted the SQLWCHAR buffer as bytes and kept the low byte of each UTF-16 code unit. Every character above U+00FF was therefore damaged: U+017C became '|', U+0142 became 'B', and where the low byte was not printable ASCII the character was dropped or, once napi_create_string_utf8 decoded the bytes, replaced with U+FFFD. A code unit whose low byte was 0x00 - U+0100, or a genuine U+0000 - terminated the message, because the string was built from a NUL-terminated buffer. swcvec2str now converts UTF-16 to UTF-8 properly, recombining surrogate pairs where SQLWCHAR is two bytes and leaving code points alone where it is four, so unixODBC and iODBC are both handled. A lone surrogate becomes U+FFFD rather than corrupting the rest of the line. Trailing NULs are trimmed, which keeps the three callers that pass a buffer size rather than a string length correct, while an embedded NUL no longer takes the tail of a message with it. The length is clamped to size(), and trim() derives it from size() rather than capacity(), so neither can read past the elements that exist. read_errors sized its buffer at 10 * 1024 and ignored msg_len, which reports the length available rather than the length written, so any longer record arrived cut to 10239 characters with no error at all. It now re-fetches a record that did not fit. `print cast(substring(@SQL, 1, 16000) as ntext)` is the documented way around PRINT's own 4000/8000 limit, so records past 10240 are ordinary, and a generated statement 5761 characters short still looks like a statement. The regrown buffer is capped at 32767 because BufferLength is a SQLSMALLINT: 40000 overflows it and the driver answers SQL_ERROR having written nothing. msg.capacity() was also being passed as BufferLength, which promised the driver more room than the vector was guaranteed to have; it now passes msg.size(). Reported and diagnosed by @FilipB97, whose patch this is. Fixes #446 Co-authored-by: FilipB97 <FilipB97@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FS9t73svfSHXWoBUehTKA2
Two layers, because the defect had both a pure-conversion half and a buffer-management half. test/diagnostic-text.test.js drives a real server: non-ASCII text in a PRINT, characters whose low byte is a control code, U+0100 not ending the message, a PRINT longer than the old 10240-unit buffer, and non-ASCII in a RAISERROR. All five fail on the previous commit's parent and pass on it. test/cpp/unit/diagnostic_text_test.cpp needs neither a server nor a driver: the conversion is pure, and the truncation case drives read_errors through a mocked SQLGetDiagRecW that reports the length available and writes only what fits, the way the real driver does. Seven of its thirteen cases fail before the fix. The long-message test is pinned at 12000 characters rather than the 16000 from the report. Measured against ODBC Driver 17 and 18, SQLGetDiagRecW will not return more than 16056 characters for one record - the 54-character "[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]" prefix plus 16002 - and reports that same 16056 whether the server PRINTed 16005, 20005 or 40005 characters, at any buffer size. That ceiling is the driver's, so the test stays below it and asserts an exact round trip instead. Both source files are free of non-ASCII characters, spelling the text under test with \u escapes and the expectations with the UTF-8 bytes they must produce, so no assertion depends on which charset a compiler or editor guesses for the file. The C++ test is its own CMake target over Logger.cpp and mock_odbc_api.h rather than a case inside cpp_tests, because cpp_tests cannot currently build: its Release config combines /GL with WINDOWS_EXPORT_ALL_SYMBOLS, which leaves cmake -E __create_def unable to read the LTCG objects, and mocks/include/mock_odbc_statement.h is out of date against the present IOdbcStatement. Neither is touched here. mock_odbc_api.h had drifted too, and gains the four IOdbcApi methods it was missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FS9t73svfSHXWoBUehTKA2
The test workflow has never compiled the C++ it is meant to be testing. `npm install` runs this package's own install script, `prebuild-install || node-gyp rebuild`, and prebuild-install succeeds on every platform in the matrix: the release assets are napi-v8 builds, which match any Node version, so all eight jobs quietly downloaded the last *published* binary and ran the suite against that. The JavaScript came from the branch and the addon did not. A C++ fix therefore looks broken in CI and a C++ regression looks fine - the first of those is how this surfaced, with the issue 446 tests failing on exactly the symptoms the commit before this one removes, because the binary under test predated it. `--build-from-source` makes prebuild-install decline with exit 1 so the `||` fallback compiles this checkout. It is scoped to this package by name, since the bare flag is read by every dependency that uses prebuild-install. Verified by running the workflow's own command locally: prebuild-install declines, node-gyp compiles, and the five issue 446 tests that fail against the published binary pass against the compiled one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FS9t73svfSHXWoBUehTKA2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The three claims — all confirmed
Narrowing. cpp/include/common/odbc_common.h:150 did exactly what he says. Live output before the fix:
--lista producent<U+FFFD>w u|ytkownika, wydajno[ciowych, podwBadnych
ł→B, ż→|, ś→[, and ó's lone 0xF3 → U+FFFD. ąćę came through as \x05 \x07 \x19 — invisible, as he warned. His premise that UTF-8 is the right target checks out: js_object_mapper.cpp:387 hands the message to Napi::String::New, i.e. napi_create_string_utf8.
NUL truncation. print N'PRZED' + nchar(0x0100) + N'PO' arrived as PRZED. Worth noting the mechanism in his own repro isn't an embedded NUL — it's U+0100's low byte being 0x00. Both truncate, and the fix handles both.
The 10239 ceiling. Exact. My direct ODBC probe, on the real driver:
replicate=16000 (PRINTed 16005 chars)
buffer 10240 (old) rc=1(WITH_INFO) msg_len=16056 written=10239
buffer 32767 (API max) rc=0 msg_len=16056 written=16056
The driver reports 16056 available and writes 10239; the old code ignored the return.
One correction to the report
His expected 16005 isn't attainable. Driver 17/18 caps a single diagnostic record at 16056 characters — the 54-char [Microsoft][ODBC Driver 17...] prefix plus 16002. msg_len is 16056 whether the server PRINTs 16005, 20005 or 40005 characters, at any buffer size. So after his fix you get 16056, i.e. everything available; the residual 3 characters are the driver's, not recoverable.
His 32767 clamp also turns out to be necessary, not just tidy: BufferLength=40000 overflows the SQLSMALLINT and the driver returns SQL_ERROR with nothing written.
Fix review
Applies cleanly to master with git apply, compiles, clang-format clean, 733 passing / 0 failing on the full JS suite. It also closes a latent overrun — msg.capacity() was being passed as BufferLength. Three minor notes:
Regression tests added