Skip to content

FIX: prevent native log format-string injection - #791

Open
Sumit Sarabhai (sumitmsft) wants to merge 2 commits into
mainfrom
sumitsar/fix-vfind-149-format-string
Open

Sumit Sarabhai (sumitmsft) wants to merge 2 commits into
mainfrom
sumitsar/fix-vfind-149-format-string

Conversation

@sumitmsft

@sumitmsft Sumit Sarabhai (sumitmsft) commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

AB#48241


Summary

Prevents server-controlled Arrow column metadata from being interpreted as a native printf-style format string.

Converts all dynamic native LOG calls to literal formats, fixes existing format/argument mismatches exposed by compiler checking, and enables compile-time printf validation for GCC, Clang, and AppleClang.

Adds a source-contract regression test that rejects nonliteral native LOG formats across first-party C++ sources and headers.

Validation

  • Windows x64 native extension build completed with 0 errors.
  • 37 no-database dependency and logging-security tests passed; 3 platform-specific tests skipped.
  • Rebuilt package import passed.
  • Python formatting and diff checks passed.
  • Independent rubber-duck review completed with no remaining findings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 08:31
@github-actions github-actions Bot added the pr-size: medium Moderate update size label Sep 17, 2026
Comment thread mssql_python/pybind/logger_bridge.hpp Fixed

Copilot AI left a comment

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.

🟢 Approval recommended

No unresolved blocking issues were identified.

Pull request overview

Prevents native printf-style log injection from server-controlled metadata and adds compile-time validation.

Changes:

  • Converts dynamic log calls to literal formats.
  • Fixes format mismatches.
  • Adds compiler checks and regression tests.
File summaries
File Description
tests/test_039_native_logging_format_security.py Adds logging format-security regression tests.
mssql_python/pybind/logger_bridge.hpp Adds printf-format annotations.
mssql_python/pybind/ddbc_bindings.cpp Secures and corrects native log calls.
mssql_python/pybind/CMakeLists.txt Enables format diagnostics.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

50%


🎯 Overall Coverage

83%


📈 Total Lines Covered: 8437 out of 10091
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/pybind/ddbc_bindings.cpp (50.0%): Missing lines 2750-2751,2778-2779

Summary

  • Total: 8 lines
  • Missing: 4 lines
  • Coverage: 50%

mssql_python/pybind/ddbc_bindings.cpp

Lines 2746-2755

  2746                             if (PyBytes_GET_SIZE(b.ptr()) != 16) {
  2747                                 LOG("BindParameterArray: GUID bytes wrong "
  2748                                     "length - param_index=%d, row=%zu, "
  2749                                     "length=%lld",
! 2750                                     paramIndex, i,
! 2751                                     static_cast<long long>(PyBytes_GET_SIZE(b.ptr())));
  2752                                 ThrowStdException("UUID binary data must be "
  2753                                                   "exactly 16 bytes long.");
  2754                             }
  2755                             std::memcpy(uuid_bytes.data(), PyBytes_AS_STRING(b.ptr()), 16);

Lines 2774-2783

  2774                         std::memcpy(guidArray[i].Data4, uuid_bytes.data() + 8, 8);
  2775                         strLenOrIndArray[i] = sizeof(SQLGUID);
  2776                     }
  2777                     LOG("BindParameterArray: SQL_C_GUID bound - "
! 2778                         "param_index=%d, count=%zu",
! 2779                         paramIndex, paramSetSize);
  2780                     dataPtr = guidArray;
  2781                     bufferLength = sizeof(SQLGUID);
  2782                     break;
  2783                 }


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.py_type_cache.hpp: 91.6%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 17, 2026 10:21
@sumitmsft
Sumit Sarabhai (sumitmsft) marked this pull request as ready for review September 17, 2026 10:27

Copilot AI left a comment

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.

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: medium Moderate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants