894: Fix error-reporting crash in non-blocking connection. (#895)

* 894: Fix error-reporting crash in non-blocking connection. Fixes: #894 If construction of a non-blocking `connection` failed for a reason _other_ than an out-of-memory error, then reporting of the error would fail. This happened because the error-reporting code tried to read the error message _after it had already cleared the connection pointer._ This means reading deallocated memory. * Drop obsolete option in CircleCI build. * Fix `assume()` chneck for clang C++17/C++20 build. In clang 19, all of a sudden the compile-time feature check for `[[assume()]]` support started issuing a warning: it says that attribute is a "C++23 extension." And the maintainer-mode build treats warnings as wrrors, so... Kaboom. The problem is that in this mode, clang 19 does define the feature test macro for this attribute even in C++ 17, but when you actually _use_ the attribute, clang warns (at least with strict checking options) that it is not part of C++17 (or C++20, as the case may be). To get around that, I wrote a more substantial feature check and ran it at _configure_ time. This is where my work on systematising the feature check mechanism really pays off — a few years ago this would have been a fairly substantial change, with lots of opportunities to do it wrong. * Work around clang compile problem in a test. For some reason the line in this unit test where we create a `std::vector<pqxx::binarystring>` triggers a deprecation warning, even though the line is bracketed in an "ignore deprecations" block. Since the whole class is deprecated, it wasn't really worth fixing properly. I'll just stop building this test on clang, until the time comes to drop the whole class and its tests anyway. Which I hope is not too far off.
jtv · Oct 5, 2024 · 7287495 · 7287495
1 parent 793fbfe
commit 7287495
Show file tree

Hide file tree

Showing 11 changed files with 64 additions and 10 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -43,7 +43,6 @@ jobs:
  name: Configure
  command: |
  ./configure \
- --disable-documentation \
  --enable-maintainer-mode \
  --enable-audit \
  --enable-shared --disable-static \

diff --git a/NEWS b/NEWS
@@ -4,6 +4,7 @@
  - Don't call`/bin/true`; macOS doesn't have it. Just call `true`. (#885)
  - Deprecate `exec_prepared()`; just use `exec()` with a `pqxx::prepped`.
  - Deprecate `exec1()` etc. Use `result::expect_rows()` etc.
+ - Fix error-reporting crash in non-blocking connection constructor. (#894)
 7.9.2
  - Fix CMake documentation install. (#848)
  - More CMake build fix. (#869)

diff --git a/cmake/pqxx_cxx_feature_checks.cmake b/cmake/pqxx_cxx_feature_checks.cmake
@@ -1,5 +1,9 @@
 # Configuration for feature checks. Generated by generate_check_config.py.
 include(CheckCXXSourceCompiles)
+try_compile(
+ PQXX_HAVE_ASSUME ${PROJECT_BINARY_DIR}
+ SOURCES ${PROJECT_SOURCE_DIR}/config-tests/PQXX_HAVE_ASSUME.cxx
+)
 try_compile(
  PQXX_HAVE_CHARCONV_FLOAT ${PROJECT_BINARY_DIR}
  SOURCES ${PROJECT_SOURCE_DIR}/config-tests/PQXX_HAVE_CHARCONV_FLOAT.cxx

diff --git a/config-tests/PQXX_HAVE_ASSUME.cxx b/config-tests/PQXX_HAVE_ASSUME.cxx
@@ -0,0 +1,5 @@
+int main(int argc, char **argv)
+{
+ [[assume(argv != nullptr)]];
+ return argc - 1;
+}
diff --git a/configitems b/configitems
@@ -4,6 +4,7 @@ PACKAGE_NAME internal autotools
 PACKAGE_STRING internal autotools
 PACKAGE_TARNAME internal autotools
 PACKAGE_VERSION internal autotools
+PQXX_HAVE_ASSUME public compiler
 PQXX_HAVE_CHARCONV_INT internal compiler
 PQXX_HAVE_CHARCONV_FLOAT internal compiler
 PQXX_HAVE_CMP public compiler

diff --git a/configure b/configure
@@ -17223,6 +17223,34 @@ fi # End of gcc-specific part.
 
 
 # Configuration for feature checks. Generated by generate_check_config.py.
+{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking PQXX_HAVE_ASSUME" >&5
+printf %s "checking PQXX_HAVE_ASSUME... " >&6; }
+PQXX_HAVE_ASSUME=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+int main(int argc, char **argv)
+
+{
+
+ [[assume(argv != nullptr)]];
+
+ return argc - 1;
+
+}
+
+
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"
+then :
+
+printf "%s\n" "#define PQXX_HAVE_ASSUME 1" >>confdefs.h
+
+else $as_nop
+ PQXX_HAVE_ASSUME=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.beam conftest.$ac_ext
+{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $PQXX_HAVE_ASSUME" >&5
+printf "%s\n" "$PQXX_HAVE_ASSUME" >&6; }
 { printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking PQXX_HAVE_CHARCONV_FLOAT" >&5
 printf %s "checking PQXX_HAVE_CHARCONV_FLOAT... " >&6; }
 PQXX_HAVE_CHARCONV_FLOAT=yes

diff --git a/include/pqxx/config.h.in b/include/pqxx/config.h.in
@@ -6,7 +6,7 @@
 /* Define to 1 if you have the <inttypes.h> header file. */
 #undef HAVE_INTTYPES_H
 
-/* Define to 1 if you have the `pq' library (-lpq). */
+/* Define to 1 if you have the 'pq' library (-lpq). */
 #undef HAVE_LIBPQ
 
 /* Define to 1 if you have the <stdint.h> header file. */
@@ -57,6 +57,9 @@
 /* Define to the version of this package. */
 #undef PACKAGE_VERSION
 
+/* Define if this feature is available. */
+#undef PQXX_HAVE_ASSUME
+
 /* Define if this feature is available. */
 #undef PQXX_HAVE_CHARCONV_FLOAT
 
@@ -114,7 +117,7 @@
 /* Define if this feature is available. */
 #undef PQXX_HAVE_YEAR_MONTH_DAY
 
-/* Define to 1 if all of the C90 standard headers exist (not just the ones
+/* Define to 1 if all of the C89 standard headers exist (not just the ones
  required in a freestanding environment). This macro is provided for
  backward compatibility; new code need not use it. */
 #undef STDC_HEADERS

diff --git a/include/pqxx/internal/header-pre.hxx b/include/pqxx/internal/header-pre.hxx
@@ -181,11 +181,8 @@
 
 
 // C++23: Assume support.
-// C++20: Assume __has_cpp_attribute is defined.
-#if !defined(__has_cpp_attribute)
-# define PQXX_ASSUME(condition) while (false)
-#elif !__has_cpp_attribute(assume)
-# define PQXX_ASSUME(condition) while (false)
-#else
+#if defined(PQXX_HAVE_ASSUME)
 # define PQXX_ASSUME(condition) [[assume(condition)]]
+#else
+# define PQXX_ASSUME(condition) while (false)
 #endif
diff --git a/pqxx_cxx_feature_checks.ac b/pqxx_cxx_feature_checks.ac
@@ -1,4 +1,14 @@
 # Configuration for feature checks. Generated by generate_check_config.py.
+AC_MSG_CHECKING([PQXX_HAVE_ASSUME])
+PQXX_HAVE_ASSUME=yes
+AC_COMPILE_IFELSE(
+ [read_test(PQXX_HAVE_ASSUME.cxx)],
+ AC_DEFINE(
+ [PQXX_HAVE_ASSUME],
+ 1,
+ [Define if this feature is available.]),
+ PQXX_HAVE_ASSUME=no)
+AC_MSG_RESULT($PQXX_HAVE_ASSUME)
 AC_MSG_CHECKING([PQXX_HAVE_CHARCONV_FLOAT])
 PQXX_HAVE_CHARCONV_FLOAT=yes
 AC_COMPILE_IFELSE(

diff --git a/src/connection.cxx b/src/connection.cxx
@@ -115,9 +115,10 @@ pqxx::connection::connection(
  throw std::bad_alloc{};
  if (status() == CONNECTION_BAD)
  {
+ std::string const msg{PQerrorMessage(m_conn)};
  PQfinish(m_conn);
  m_conn = nullptr;
- throw pqxx::broken_connection{PQerrorMessage(m_conn)};
+ throw pqxx::broken_connection{msg};
  }
 }
 

diff --git a/test/unit/test_binarystring.cxx b/test/unit/test_binarystring.cxx
@@ -166,6 +166,10 @@ void test_binarystring_stream()
 
 void test_binarystring_array_stream()
 {
+ // This test won't compile on clang in maintainer mode. For some reason,
+ // clang seems to ignore the ignore-deprecated headers in just this one
+ // function, where we create the vector of binarystring.
+#if !defined(__clang__)
  pqxx::connection cx;
  pqxx::transaction tx{cx};
  tx.exec("CREATE TEMP TABLE pqxxbinstream(id integer, vec bytea[])")
@@ -201,6 +205,7 @@ void test_binarystring_array_stream()
  tx.query_value<std::size_t>(
  "SELECT octet_length(vec[1]) FROM pqxxbinstream"),
  std::size(data1), "Bytea length broke inside array.");
+#endif // __clang__
 }