From eee0ace49ed4f2d83936bc3452a79d51eec2c3f5 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Thu, 17 Sep 2026 09:31:23 +0100 Subject: [PATCH 1/2] FIX: prevent native log format-string injection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/CMakeLists.txt | 5 +- mssql_python/pybind/ddbc_bindings.cpp | 19 +++--- mssql_python/pybind/logger_bridge.hpp | 15 ++++- ...test_039_native_logging_format_security.py | 61 +++++++++++++++++++ 4 files changed, 88 insertions(+), 12 deletions(-) create mode 100644 tests/test_039_native_logging_format_security.py diff --git a/mssql_python/pybind/CMakeLists.txt b/mssql_python/pybind/CMakeLists.txt index 2ce264253..77d599bd5 100644 --- a/mssql_python/pybind/CMakeLists.txt +++ b/mssql_python/pybind/CMakeLists.txt @@ -370,10 +370,13 @@ if(MSVC) endif() # Add warning flags for GCC/Clang on Linux and macOS -if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang") +if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" OR + CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang") target_compile_options(ddbc_bindings PRIVATE -Werror # Treat warnings as errors -Wattributes # Enable attribute warnings (cross-compiler) + -Wformat=2 # Check nonliteral formats and printf argument types + -Werror=format-security # Explicitly keep format-security diagnostics fatal ) # GCC-specific warning flags diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 35a3b6da4..db630f2a9 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -2746,8 +2746,9 @@ SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& if (PyBytes_GET_SIZE(b.ptr()) != 16) { LOG("BindParameterArray: GUID bytes wrong " "length - param_index=%d, row=%zu, " - "length=%d", - paramIndex, i, PyBytes_GET_SIZE(b.ptr())); + "length=%lld", + paramIndex, i, + static_cast(PyBytes_GET_SIZE(b.ptr()))); ThrowStdException("UUID binary data must be " "exactly 16 bytes long."); } @@ -2774,8 +2775,8 @@ SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& strLenOrIndArray[i] = sizeof(SQLGUID); } LOG("BindParameterArray: SQL_C_GUID bound - " - "param_index=%d, null=%zu, bytes=%zu, uuid_obj=%zu", - paramIndex); + "param_index=%d, count=%zu", + paramIndex, paramSetSize); dataPtr = guidArray; bufferLength = sizeof(SQLGUID); break; @@ -4357,7 +4358,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum } if (dataLen == SQL_NO_TOTAL) { LOG("Cannot determine the length of the data. Returning NULL " - "value instead. Column ID - {}", + "value instead. Column ID - %d", col); Py_INCREF(Py_None); PyList_SET_ITEM(row, col - 1, Py_None); @@ -4383,7 +4384,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum if (dataLen == 0) { // Handle zero-length (non-NULL) data for complex types LOG("Column data length is 0 for complex datatype. Setting " - "None to the result row. Column ID - {}", + "None to the result row. Column ID - %d", col); Py_INCREF(Py_None); PyList_SET_ITEM(row, col - 1, Py_None); @@ -4418,7 +4419,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum } catch (const py::error_already_set& e) { // Handle the exception, e.g., log the error and set // py::none() - LOG("Error converting to decimal: {}", e.what()); + LOG("Error converting to decimal: %s", e.what()); Py_INCREF(Py_None); PyList_SET_ITEM(row, col - 1, Py_None); } @@ -5020,7 +5021,7 @@ SQLRETURN FetchArrowBatch_wrap(SqlHandlePtr StatementHandle, py::list& capsules, errorString << "Unsupported data type for Arrow batch fetch for column - " << columnName.c_str() << ", Type - " << dataType << ", column ID - " << (i + 1); - LOG(errorString.str().c_str()); + LOG("FetchArrowBatch: %s", errorString.str().c_str()); ThrowStdException(errorString.str()); break; } @@ -5545,7 +5546,7 @@ SQLRETURN FetchArrowBatch_wrap(SqlHandlePtr StatementHandle, py::list& capsules, std::ostringstream errorString; errorString << "Unsupported data type for column ID - " << (idxCol + 1) << ", Type - " << dataType; - LOG(errorString.str().c_str()); + LOG("FetchArrowBatch: %s", errorString.str().c_str()); ThrowStdException(errorString.str()); break; } diff --git a/mssql_python/pybind/logger_bridge.hpp b/mssql_python/pybind/logger_bridge.hpp index 49cfe5310..e7b8f328c 100644 --- a/mssql_python/pybind/logger_bridge.hpp +++ b/mssql_python/pybind/logger_bridge.hpp @@ -25,6 +25,13 @@ namespace py = pybind11; namespace mssql_python { namespace logging { +#if defined(__GNUC__) || defined(__clang__) +#define MSSQL_PRINTF_FORMAT(format_index, first_argument) \ + __attribute__((format(printf, format_index, first_argument))) +#else +#define MSSQL_PRINTF_FORMAT(format_index, first_argument) +#endif + // Log level constants (matching Python levels) // Note: Avoid using ERROR as it conflicts with Windows.h macro const int LOG_LEVEL_DEBUG = 10; // Debug/diagnostic logging @@ -81,7 +88,8 @@ class LoggerBridge { * @param format Printf-style format string * @param ... Variable arguments for format string */ - static void log(int level, const char* file, int line, const char* format, ...); + static void log(int level, const char* file, int line, const char* format, ...) + MSSQL_PRINTF_FORMAT(4, 5); /** * Get the current log level. @@ -124,7 +132,8 @@ class LoggerBridge { * @param args Variable arguments * @return Formatted string */ - static std::string formatMessage(const char* format, va_list args); + static std::string formatMessage(const char* format, va_list args) + MSSQL_PRINTF_FORMAT(1, 0); /** * Helper to extract filename from full path. @@ -138,6 +147,8 @@ class LoggerBridge { } // namespace logging } // namespace mssql_python +#undef MSSQL_PRINTF_FORMAT + // Convenience macros for logging // Single LOG() macro for all diagnostic logging (DEBUG level) diff --git a/tests/test_039_native_logging_format_security.py b/tests/test_039_native_logging_format_security.py new file mode 100644 index 000000000..161c19349 --- /dev/null +++ b/tests/test_039_native_logging_format_security.py @@ -0,0 +1,61 @@ +"""Regression guards for native printf-style logging.""" + +import re +from pathlib import Path + +import pytest + +_PYBIND_DIR = Path(__file__).resolve().parents[1] / "mssql_python" / "pybind" +_LOGGER_HEADER = _PYBIND_DIR / "logger_bridge.hpp" +_CMAKE = _PYBIND_DIR / "CMakeLists.txt" +pytestmark = pytest.mark.skipif( + not _PYBIND_DIR.is_dir(), + reason="requires a source checkout; isolated wheel tests omit the source tree", +) + + +def _code_without_comments(text): + text = re.sub( + r"/\*.*?\*/", + lambda match: "\n" * match.group().count("\n"), + text, + flags=re.DOTALL, + ) + return "\n".join( + "" if line.lstrip().startswith("#define LOG") else re.sub(r"//.*", "", line) + for line in text.splitlines() + ) + + +def test_native_log_calls_use_literal_format_strings(): + dynamic_calls = [] + pattern = re.compile(r"\bLOG(?:_INFO|_WARNING|_ERROR)?\s*\(\s*(.)") + paths = ( + path + for suffix in ("*.cpp", "*.hpp", "*.h") + for path in _PYBIND_DIR.rglob(suffix) + if "build" not in path.relative_to(_PYBIND_DIR).parts + ) + for path in paths: + code = _code_without_comments(path.read_text(encoding="utf-8")) + for match in pattern.finditer(code): + if match.group(1) != '"': + line_number = code.count("\n", 0, match.start()) + 1 + source_line = code.splitlines()[line_number - 1].strip() + dynamic_calls.append( + f"{path.relative_to(_PYBIND_DIR)}:{line_number}: {source_line}" + ) + + assert not dynamic_calls, f"native LOG calls must use literal format strings: {dynamic_calls}" + + +def test_logger_bridge_enables_compile_time_format_checks(): + header = _LOGGER_HEADER.read_text(encoding="utf-8") + cmake = _CMAKE.read_text(encoding="utf-8") + + assert "__attribute__((format(printf, format_index, first_argument)))" in header + assert "MSSQL_PRINTF_FORMAT(4, 5)" in header + assert "MSSQL_PRINTF_FORMAT(1, 0)" in header + assert 'CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"' in cmake + assert "-Wformat=2" in cmake + assert "-Werror=format-security" in cmake From 189cfb6ed10af944c12ff6b35747625b29ff0c98 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Thu, 17 Sep 2026 11:21:50 +0100 Subject: [PATCH 2/2] FIX: suppress false positive on printf annotation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/logger_bridge.hpp | 2 +- tests/test_039_native_logging_format_security.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/mssql_python/pybind/logger_bridge.hpp b/mssql_python/pybind/logger_bridge.hpp index e7b8f328c..4e94408ce 100644 --- a/mssql_python/pybind/logger_bridge.hpp +++ b/mssql_python/pybind/logger_bridge.hpp @@ -27,7 +27,7 @@ namespace logging { #if defined(__GNUC__) || defined(__clang__) #define MSSQL_PRINTF_FORMAT(format_index, first_argument) \ - __attribute__((format(printf, format_index, first_argument))) + __attribute__((format(printf, format_index, first_argument))) // DevSkim: ignore DS154189 #else #define MSSQL_PRINTF_FORMAT(format_index, first_argument) #endif diff --git a/tests/test_039_native_logging_format_security.py b/tests/test_039_native_logging_format_security.py index 161c19349..a357981cc 100644 --- a/tests/test_039_native_logging_format_security.py +++ b/tests/test_039_native_logging_format_security.py @@ -54,6 +54,7 @@ def test_logger_bridge_enables_compile_time_format_checks(): cmake = _CMAKE.read_text(encoding="utf-8") assert "__attribute__((format(printf, format_index, first_argument)))" in header + assert "DevSkim: ignore DS154189" in header assert "MSSQL_PRINTF_FORMAT(4, 5)" in header assert "MSSQL_PRINTF_FORMAT(1, 0)" in header assert 'CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"' in cmake