diff --git a/mssql_python/pybind/CMakeLists.txt b/mssql_python/pybind/CMakeLists.txt index 2ce26425..77d599bd 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 caaca121..8f5a4e91 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -2747,8 +2747,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."); } @@ -2775,8 +2776,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; @@ -4360,7 +4361,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); @@ -4386,7 +4387,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); @@ -4421,7 +4422,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); } @@ -5023,7 +5024,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; } @@ -5548,7 +5549,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 49cfe531..4e94408c 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))) // DevSkim: ignore DS154189 +#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 00000000..a357981c --- /dev/null +++ b/tests/test_039_native_logging_format_security.py @@ -0,0 +1,62 @@ +"""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 "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 + assert "-Wformat=2" in cmake + assert "-Werror=format-security" in cmake