Unaligned access UB in C impl parse_row_binary()

[matoro]

Hi, I identified an instance of undefined behavior due to an unaligned access in the C implementation, in the parse_row_binary() function. I've captured a backtrace from the unit test on a platform that does not permit unaligned access, and then ran the same test with UBSAN to demonstrate that the issue is present and invokes undefined behavior on all platforms. I believe this is the code at fault:

https://github.com/psycopg/psycopg/blob/3.1.17/psycopg_c/psycopg_c/_psycopg/copy.pyx#L194-L195

Complete test run: https://924180.bugs.gentoo.org/attachment.cgi?id=884637

Relevant backtrace:

(gdb) bt full 9
#0  0xfff8000100d9249c in ?? () from /lib64/libc.so.6
No symbol table info available.
#1  0xfff8000100d42eb4 in raise () from /lib64/libc.so.6
No symbol table info available.
#2  0xfff8000100780224 in faulthandler_fatal_error (signum=10) at ./Modules/faulthandler.c:384
        fd = 9
        i = 0
        handler = 0xfff8000100b2d2b0 <faulthandler_handlers>
        save_errno = 0
        found = 1
#3  <signal handler called>
No symbol table info available.
#4  __pyx_pf_9psycopg_c_8_psycopg_4parse_row_binary (__pyx_self=<_cython_3_0_8.cython_function_or_method at remote 0xfff8000107d806c0>, 
    __pyx_v_data=<memoryview at remote 0xfff800010a84ea40>, __pyx_v_tx=0xfff800010a84df00) at psycopg_c/_psycopg.c:19288
        __pyx_v_ptr = 0x1000091e6c3 ""
        __pyx_v_bufsize = 67
        __pyx_v_bufend = 0x1000091e706 ""
        __pyx_v_benfields = 24920
        __pyx_v_nfields = -524287
        __pyx_v_row = 0x0
        __pyx_v_col = -524287
        __pyx_v_belength = -524287
        __pyx_v_length = 1099519657312
        __pyx_v_field = 0x0
        __pyx_r = 0x0
        __pyx_t_1 = 0
        __pyx_t_2 = 0x0
        __pyx_t_3 = 169188592
        __pyx_t_4 = 176482864
        __pyx_t_5 = 11692416
        __pyx_t_6 = 0x0
        __pyx_t_7 = 0x0
        __pyx_t_8 = -524287
        __pyx_lineno = 0
        __pyx_filename = 0x0
        __pyx_clineno = 0
#5  0xfff800010a3196a4 in __pyx_pw_9psycopg_c_8_psycopg_5parse_row_binary (__pyx_self=<_cython_3_0_8.cython_function_or_method at remote 0xfff8000107d806c0>, __pyx_args=0xfff800010009fa90, 
    __pyx_nargs=2, __pyx_kwds=0x0) at psycopg_c/_psycopg.c:19220
        __pyx_v_data = <memoryview at remote 0xfff800010a84ea40>
        __pyx_v_tx = 0xfff800010a84df00
        __pyx_kwvalues = 0xfff800010009faa0
        values = {<memoryview at remote 0xfff800010a84ea40>, <psycopg_c._psycopg.Transformer at remote 0xfff800010a84df00>}
        __pyx_lineno = 0
        __pyx_filename = 0x0
        __pyx_clineno = 0
        __pyx_r = 0x0
#6  0xfff800010a427d0c in __Pyx_CyFunction_Vectorcall_FASTCALL_KEYWORDS (func=<_cython_3_0_8.cython_function_or_method at remote 0xfff8000107d806c0>, args=0xfff800010009fa90, 
    nargsf=9223372036854775810, kwnames=0x0) at psycopg_c/_psycopg.c:95554
        cyfunc = 0xfff8000107d806c0
        def = 0xfff800010a601f90 <__pyx_mdef_9psycopg_c_8_psycopg_5parse_row_binary>
        nargs = 2
        self = <_cython_3_0_8.cython_function_or_method at remote 0xfff8000107d806c0>
#7  0xfff80001004a2538 in _PyObject_VectorcallTstate (tstate=0xfff8000100c2e568 <_PyRuntime+166328>, callable=<_cython_3_0_8.cython_function_or_method at remote 0xfff8000107d806c0>, 
    args=0xfff800010009fa90, nargsf=9223372036854775810, kwnames=0x0) at ./Include/internal/pycore_call.h:92
        func = 0xfff800010a427c38 <__Pyx_CyFunction_Vectorcall_FASTCALL_KEYWORDS>
        res = <dict_keys at remote 0xfff8000100b00000>
        __PRETTY_FUNCTION__ = "_PyObject_VectorcallTstate"
#8  0xfff80001004a3818 in PyObject_Vectorcall (callable=<_cython_3_0_8.cython_function_or_method at remote 0xfff8000107d806c0>, args=0xfff800010009fa90, nargsf=9223372036854775810, 
    kwnames=0x0) at Objects/call.c:299
        tstate = 0xfff8000100c2e568 <_PyRuntime+166328>
(More stack frames follow...)

UBSAN run of the relevant test:

$ UBSAN_OPTIONS=halt_on_error=1 PSYCOPG_TEST_DSN="host=$(realpath ../..) dbname=test" PYTHONPATH=../psycopg-3.1.17-python3_11/install/usr/lib/python3.11/site-packages/ PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PSYCOPG_IMPL=c /usr/bin/python3.11 -m pytest -vv -ra -l -Wdefault --color=yes -o console_output_style=count -o tmp_path_retention_count=0 -o tmp_path_retention_policy=failed -p anyio tests/test_copy_async.py::test_read_rows -s
===================================================================================== test session starts ======================================================================================
platform linux -- Python 3.11.7, pytest-7.4.4, pluggy-1.3.0 -- /usr/bin/python3.11
cachedir: .pytest_cache
default selector: EpollSelector
Server version: PostgreSQL 16.2 on powerpc64le-unknown-linux-gnu, compiled by powerpc64le-unknown-linux-gnu-gcc (Gentoo 13.2.1_p20240113-r1 p12) 13.2.1 20240113, 64-bit
libpq wrapper implementation: c
libpq used: 160002
libpq compiled: 160002
rootdir: /var/tmp/portage/dev-python/psycopg-3.1.17/work/psycopg-3.1.17
configfile: pyproject.toml
plugins: anyio-4.2.0
collected 4 items                                                                                                                                                                              

tests/test_copy_async.py::test_read_rows[asyncio-names-0] PASSED
tests/test_copy_async.py::test_read_rows[asyncio-names-1] psycopg_c/_psycopg.c:19287:21: runtime error: load of misaligned address 0x000144ecf413 for type 'uint16_t', which requires 2 byte alignment
0x000144ecf413: note: pointer points here
 00  00 00 00 00 03 00 00 00  04 00 00 00 0a 00 00 00  05 68 65 6c 6c 6f 00 00  00 2c 00 00 00 01 00
              ^ 
2024-02-09 22:05:25.696 GMT [16183] LOG:  unexpected EOF on client connection with an open transaction

[dvarrazzo]

Thank you for the report.

Any proposal about how to fix it?

[dvarrazzo]

The error is at line 194:

cdef uint16_t benfields = (<uint16_t *>ptr)[0]

which translates to

  __pyx_v_benfields = (((uint16_t *)__pyx_v_ptr)[0]);

probably replacing this assignment with a memcpy?

[matoro]

The error is at line 194:

cdef uint16_t benfields = (<uint16_t *>ptr)[0]

which translates to

  __pyx_v_benfields = (((uint16_t *)__pyx_v_ptr)[0]);

probably replacing this assignment with a memcpy?

I thought this as well, but I just tried this:

diff --git a/psycopg_c/psycopg_c/_psycopg/copy.pyx b/psycopg_c/psycopg_c/_psycopg/copy.pyx
index b943095d..e1ca8191 100644
--- a/psycopg_c/psycopg_c/_psycopg/copy.pyx
+++ b/psycopg_c/psycopg_c/_psycopg/copy.pyx
@@ -191,7 +191,8 @@ def parse_row_binary(data, tx: Transformer) -> Tuple[Any, ...]:
     _buffer_as_string_and_size(data, <char **>&ptr, &bufsize)
     cdef unsigned char *bufend = ptr + bufsize
 
-    cdef uint16_t benfields = (<uint16_t *>ptr)[0]
+    cdef uint16_t benfields
+    memcpy(&benfields, ptr, sizeof(benfields))
     cdef int nfields = endian.be16toh(benfields)
     ptr += sizeof(benfields)
     cdef list row = PyList_New(nfields)

and I still get a crash. Not being able to see the actual emitted C makes it rather hard to identify.

Are you able to reproduce the problem? All it should take is compile with -fsanitize=undefined, and pass -s to pytest. You can also set UBSAN_OPTIONS=halt_on_error=1 to force it to crash as well.

[dvarrazzo]

I haven't test yet, thank you very much for the repro instructions.

And you can definitely access the C code: after running pip install ./psycopg_c you can find a fresh psycopg_c/psycopg_c/_psycopg.c.

[dvarrazzo]

And yes, can reproduce it too (on ubuntu 22.04). On master:

CFLAGS="-fsanitize=undefined" pip install ./psycopg_c/
UBSAN_OPTIONS=halt_on_error=1 pytest -s -v "tests/test_copy_async.py::test_copy_out_allchars[asyncio-Format.BINARY]"
...
libpq wrapper implementation: c
libpq used: 160001
libpq compiled: 160001
...
tests/test_copy_async.py::test_copy_out_allchars[asyncio-Format.BINARY] psycopg_c/_psycopg.c:19974:21: runtime error: load of misaligned address 0x559190304133 for type 'uint16_t', which requires 2 byte alignment
0x559190304133: note: pointer points here
 00  00 00 00 00 01 00 00 00  01 01 00 00 91 55 00 00  00 00 00 00 00 00 00 00  51 02 00 00 00 00 00
              ^ 

[dvarrazzo]

Working on fixing it in #735