Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions .github/workflows/build-and-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ jobs:
fail-fast: false
matrix:
include:
# ── Windows x64 native ─────────────────────────────────────────────
# ── Windows x64 native ─────────────────────────────────────────────
- os: windows-2022
artifact-name: windows-x64-binaries
firebird-version: '5.0.3'
- os: windows-2022
firebird-branch: master

# ── Windows x86 native (WoW64) ─────────────────────────────────────
# ── Windows x86 native (WoW64) ─────────────────────────────────────
- os: windows-2022
artifact-name: windows-x86-binaries
arch: Win32
Expand All @@ -35,26 +35,34 @@ jobs:
arch: Win32
firebird-branch: master

# ── Windows ARM64 native ───────────────────────────────────────────
# ── Windows ARM64 native ─────────────────────────────────────────────
# Official Firebird releases have no win-arm64 binaries; snapshots do.
- os: windows-11-arm
artifact-name: windows-arm64-binaries
firebird-branch: master

# ── Linux x64 native ───────────────────────────────────────────────
# ── Linux x64 native ─────────────────────────────────────────────────
- os: ubuntu-22.04
artifact-name: linux-x64-binaries
firebird-version: '5.0.3'
- os: ubuntu-22.04
firebird-branch: master

# ── Linux ARM64 native ─────────────────────────────────────────────
# ── Linux ARM64 native ─────────────────────────────────────────────
- os: ubuntu-22.04-arm
artifact-name: linux-arm64-binaries
firebird-version: '5.0.3'
- os: ubuntu-22.04-arm
firebird-branch: master

# ── Linux x64 sanitizers (Debug, Firebird 5.0.3) ─────────────────────
- os: ubuntu-22.04
sanitizer: Asan
firebird-version: '5.0.3'
- os: ubuntu-22.04
sanitizer: Valgrind
firebird-version: '5.0.3'

runs-on: ${{ matrix.os }}

steps:
Expand All @@ -70,6 +78,10 @@ jobs:
if: runner.os == 'Linux'
run: sudo apt-get update && sudo apt-get install -y unixodbc unixodbc-dev

- name: Install Valgrind
if: matrix.sanitizer == 'Valgrind'
run: sudo apt-get install -y valgrind

- name: Build, install and test
shell: pwsh
env:
Expand All @@ -79,7 +91,10 @@ jobs:
run: |
$archArgs = @{}
if ('${{ matrix.arch }}') { $archArgs['Architecture'] = '${{ matrix.arch }}' }
Invoke-Build test -Configuration Release @archArgs -File ./firebird-odbc-driver.build.ps1
$sanitizerArgs = @{}
if ('${{ matrix.sanitizer }}') { $sanitizerArgs['Sanitizer'] = '${{ matrix.sanitizer }}' }
$config = if ('${{ matrix.sanitizer }}') { 'Debug' } else { 'Release' }
Invoke-Build test -Configuration $config @archArgs @sanitizerArgs -File ./firebird-odbc-driver.build.ps1

- name: Upload artifacts (Windows)
if: runner.os == 'Windows' && matrix.artifact-name
Expand Down
50 changes: 43 additions & 7 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,41 @@ set(CMAKE_C_STANDARD_REQUIRED ON)
option(BUILD_SHARED_LIBS "Build shared libraries" ON)
option(BUILD_TESTING "Build tests" ON)

# ---------------------------------------------------------------------------
# Sanitizer options
# ---------------------------------------------------------------------------
option(BUILD_WITH_ASAN "Enable AddressSanitizer (-fsanitize=address)" OFF)
option(BUILD_WITH_VALGRIND "Enable Valgrind memcheck via CTest" OFF)

if(BUILD_WITH_ASAN AND BUILD_WITH_VALGRIND)
message(FATAL_ERROR "BUILD_WITH_ASAN and BUILD_WITH_VALGRIND are mutually exclusive. "
"ASAN instruments the binary at compile time; Valgrind instruments at runtime. "
"They must not be combined.")
endif()

if(MSVC AND (BUILD_WITH_ASAN OR BUILD_WITH_VALGRIND))
message(WARNING "Sanitizers are not yet supported on MSVC. "
"BUILD_WITH_ASAN / BUILD_WITH_VALGRIND will be ignored.")
elseif(NOT MSVC)
if(BUILD_WITH_ASAN)
message(STATUS "AddressSanitizer: ENABLED")
add_compile_options(-fsanitize=address -fno-omit-frame-pointer)
add_link_options(-fsanitize=address)
endif()

if(BUILD_WITH_VALGRIND)
find_program(VALGRIND_COMMAND valgrind)
if(NOT VALGRIND_COMMAND)
message(FATAL_ERROR "Valgrind not found but BUILD_WITH_VALGRIND is ON. "
"Install it with: sudo apt-get install valgrind")
endif()
message(STATUS "Valgrind memcheck: ENABLED (${VALGRIND_COMMAND})")
set(MEMORYCHECK_COMMAND ${VALGRIND_COMMAND})
set(MEMORYCHECK_COMMAND_OPTIONS
"--leak-check=full --error-exitcode=1 --suppressions=${CMAKE_SOURCE_DIR}/valgrind.supp")
endif()
endif()

# ---------------------------------------------------------------------------
# CPU architecture detection (from PR #248)
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -95,12 +130,19 @@ else()

# Compiler optimization flags (from PR #248 / old makefile.linux)
add_compile_options(
"$<$<CONFIG:Debug>:-O0;-g3;-D_DEBUG;-DDEBUG;-DLOGGING;-fexceptions>"
"$<$<CONFIG:Debug>:-O0;-g3;-D_DEBUG;-fexceptions>"
"$<$<CONFIG:Release>:-O3;-DNDEBUG;-ftree-loop-vectorize>"
"$<$<CONFIG:RelWithDebInfo>:-O2;-g;-DNDEBUG>"
"$<$<CONFIG:MinSizeRel>:-Os;-DNDEBUG>"
)

# Debug macros: DEBUG and LOGGING are for Debug builds only;
# sanitizer builds strip them for cleaner output (less noise in reports).
if(CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT BUILD_WITH_ASAN AND NOT BUILD_WITH_VALGRIND)
add_definitions(-DDEBUG)
add_definitions(-DLOGGING)
endif()

# SSE4.1 for x86/x86_64 (from PR #248)
if(FBODBC_ARCH STREQUAL "x86" OR FBODBC_ARCH STREQUAL "i686"
OR FBODBC_ARCH STREQUAL "x86_64" OR FBODBC_ARCH STREQUAL "AMD64")
Expand Down Expand Up @@ -246,12 +288,6 @@ else()
)
endif()

# Debug-specific definitions (matching .vcxproj: DEBUG;LOGGING for Debug configs)
target_compile_definitions(OdbcFb PRIVATE
$<$<CONFIG:Debug>:DEBUG>
$<$<CONFIG:Debug>:LOGGING>
Comment thread
fdcastel marked this conversation as resolved.
)

# ---------------------------------------------------------------------------
# Testing
# ---------------------------------------------------------------------------
Expand Down
60 changes: 57 additions & 3 deletions CMakePresets.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,47 @@
"description": "Default preset — uses 'build/' as the binary directory so that all tools (command-line, Visual Studio, CLion, VS Code) share the same layout.",
"binaryDir": "${sourceDir}/build",
"cacheVariables": {
"CMAKE_INSTALL_PREFIX": "${sourceDir}/install"
"CMAKE_INSTALL_PREFIX": "${sourceDir}/install",
"BUILD_WITH_ASAN": "OFF",
"BUILD_WITH_VALGRIND": "OFF"
}
},
{
"name": "release",
"displayName": "Release",
"inherits": "default",
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Release"
"CMAKE_BUILD_TYPE": "Release",
"BUILD_WITH_ASAN": "OFF",
"BUILD_WITH_VALGRIND": "OFF"
}
},
{
"name": "debug",
"displayName": "Debug",
"inherits": "default",
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug"
"CMAKE_BUILD_TYPE": "Debug",
"BUILD_WITH_ASAN": "OFF",
"BUILD_WITH_VALGRIND": "OFF"
}
},
{
"name": "asan",
"displayName": "Debug + AddressSanitizer",
"inherits": "debug",
"cacheVariables": {
"BUILD_WITH_ASAN": "ON",
"BUILD_WITH_VALGRIND": "OFF"
}
},
{
"name": "valgrind",
"displayName": "Debug + Valgrind",
"inherits": "debug",
"cacheVariables": {
"BUILD_WITH_VALGRIND": "ON",
"BUILD_WITH_ASAN": "OFF"
}
}
],
Expand All @@ -42,6 +66,16 @@
"name": "debug",
"configurePreset": "default",
"configuration": "Debug"
},
{
"name": "asan",
"configurePreset": "asan",
"configuration": "Debug"
},
{
"name": "valgrind",
"configurePreset": "valgrind",
"configuration": "Debug"
}
],
"testPresets": [
Expand All @@ -51,6 +85,26 @@
"output": {
"outputOnFailure": true
}
},
{
"name": "asan",
"configurePreset": "asan",
"output": {
"outputOnFailure": true
},
"environment": {
"ASAN_OPTIONS": "detect_leaks=1:halt_on_error=1:print_stats=1"
}
},
{
"name": "valgrind",
"configurePreset": "valgrind",
"output": {
"outputOnFailure": true
},
"execution": {
"timeout": 600
}
}
]
}
68 changes: 60 additions & 8 deletions MainUnicode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,21 @@
extern FILE *logFile;
using namespace OdbcJdbcLibrary;

#ifndef _WINDOWS
// SQLWCHAR-aware length (in SQLWCHAR units), safe on Linux where
// sizeof(wchar_t) != sizeof(SQLWCHAR). Do NOT use wcslen() on SQLWCHAR
// data on Linux — it reads two SQLWCHARs per wchar_t and runs off the end.
static size_t sqlwcharLen( const SQLWCHAR *s )
{
size_t n = 0;
if ( !s )
return 0;
while ( s[n] )
++n;
return n;
}
#endif

#ifdef _WINDOWS
extern UINT codePage; // from Main.cpp
#endif
Expand Down Expand Up @@ -85,7 +100,7 @@ class ConvertingString
if ( length == SQL_NTS )
lengthString = 0;
else if ( retCountOfBytes )
lengthString = length / sizeof(wchar_t);
lengthString = length / sizeof(SQLWCHAR);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please! Have you forgotten that this change causes a stack smash?)) Simply because the the lengthString becomes greater, and the mbstowcs call below begins to break the client's stack, overflowing the unicodeString, passed by the pointer to the ODBC call?

This undoubtedly right change CAN NOT be done without a deep refactoring of the unicode routins. I thought I've explaned it clearly in #289 (comment)

@fdcastel fdcastel Apr 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right — this change alone breaks the symmetric mbstowcs((wchar_t*)unicodeString, ..., lengthString) call in the destructor, which then writes lengthString * sizeof(wchar_t) bytes into the caller's SQLWCHAR buffer and smashes the stack. Fixing the capacity without also rewriting the write-back path is not viable. Reverted in f20e6fc; the ConvertingString / mbstowcs refactor has been tracked as Tier 9.1 in #287. PR #289 is now strictly scoped to the ASAN/Valgrind CI plumbing plus the safe internal-buffer allocation fix in 832d8e7.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But it's insufficient to close this thread, please revert the changes) return the line 88 back to the original
lengthString = length / sizeof(wchar_t);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted in f20e6fc;

Cannot see where the line 88 is reverted in ^^^

else
lengthString = length;
}
Expand Down Expand Up @@ -135,13 +150,33 @@ class ConvertingString
if ( len > 0 )
len--;
#else
len = mbstowcs( (wchar_t*)unicodeString, (const char*)byteString, lengthString );
// SQLWCHAR is 2 bytes on Linux (unixODBC defines it as unsigned short),
// but wchar_t is 4 bytes, so mbstowcs((wchar_t*)unicodeString, ...)
// both corrupts the output and risks overflowing the caller's buffer.
// Widen byte-by-byte into SQLWCHAR units, matching what unixODBC's
// ansi_to_unicode_copy() does internally. This is correct for the
// ASCII-only error/state strings that reach this code path; non-ASCII
// input will be handled by the broader ConvertingString rewrite tracked
// in issue #287 (Tier 9.1).
{
const SQLCHAR *src = byteString;
size_t i = 0;
while ( i < (size_t)lengthString && src[i] != 0 )
{
unicodeString[i] = (SQLWCHAR)( src[i] & 0xFF );
++i;
}
len = i;
}
#endif
}

if ( len > 0 )
{
*(LPWSTR)(unicodeString + len) = L'\0';
// NUL-terminate in SQLWCHAR units. LPWSTR assignment of L'\0' writes
Comment thread
fdcastel marked this conversation as resolved.
Outdated
// sizeof(wchar_t) bytes, which overruns the output buffer by 2 bytes
// on Linux.
unicodeString[len] = 0;

if ( realLength )
{
Expand Down Expand Up @@ -170,12 +205,18 @@ class ConvertingString
wchar_t saveWC;

if ( length == SQL_NTS )
#ifdef _WINDOWS
length = (int)wcslen( (const wchar_t*)wcString );
else if ( wcString[length] != L'\0' )
#else
length = (int)sqlwcharLen( wcString );
#endif
else if ( wcString[length] != 0 )
{
ptEndWC = (wchar_t*)&wcString[length];
saveWC = *ptEndWC;
*ptEndWC = L'\0';
// Write a SQLWCHAR-sized NUL so we don't overrun the input by 2 bytes
// on Linux (wchar_t is 4 bytes there).
wcString[length] = 0;
}

if ( connection )
Expand All @@ -185,7 +226,10 @@ class ConvertingString
#ifdef _WINDOWS
bytesNeeded = WideCharToMultiByte( codePage, (DWORD)0, wcString, length, NULL, (int)0, NULL, NULL );
#else
bytesNeeded = wcstombs( NULL, (const wchar_t*)wcString, length );
// See the symmetric comment in the destructor above: wcstombs assumes
// wchar_t-sized input, which corrupts SQLWCHAR data on Linux. The
// byte-narrowing loop below produces exactly `length` output bytes.
bytesNeeded = (size_t)length;
#endif
}

Expand All @@ -198,7 +242,15 @@ class ConvertingString
#ifdef _WINDOWS
bytesNeeded = WideCharToMultiByte( codePage, 0, wcString, length, (LPSTR)byteString, (int)bytesNeeded, NULL, NULL );
#else
bytesNeeded = wcstombs( (char *)byteString, (const wchar_t*)wcString, bytesNeeded );
{
size_t i = 0;
while ( i < (size_t)length && wcString[i] != 0 )
{
byteString[i] = (SQLCHAR)( wcString[i] & 0xFF );
++i;
}
bytesNeeded = i;
}
#endif
}

Expand All @@ -220,7 +272,7 @@ class ConvertingString
if ( lengthString )
{
byteString = new SQLCHAR[ lengthString + 2 ];
memset(byteString, 0, lengthString + 2);
memset( byteString, 0, lengthString + 2 );
}
else
byteString = NULL;
Expand Down
Loading