gh-158583: Fix uninitialized memory read in bytes.fromhex()

[vstinner]

• Issue: bytes.fromhex() reads uninitialized memory #158583

[vstinner]

cc @StanFromIreland

bytes.fromhex() was modified in Python 3.14 to accept more types; Python 3.13 only accepts str. So the undefined behavior was introduced in Python 3.14. So far, before me running Valgrind, nobody reported the issue. So I'm not sure if it's needed to add a Changelog entry.

The modified code is tested by BytesTest.test_fromhex() and ByteArrayTest.test_fromhex() of test_bytes. For example, running the test in Valgrind reports the issue.

[vstinner]

In Python 3.13, reading str[hexlen] is fine because str object always allocate a trailing null character: https://docs.python.org/dev/c-api/unicode.html.

The internal buffer always includes an extra trailing null character for compatibility with null terminated C strings.

[vstinner]

I'm also curious why only Valgrind reports the issue. In the CI, we are running the Python test suite on Python built with Address Sanitizer. This build doesn't detect usage of uninitialized memory?

[encukou]

Apparently, when array.array() created from bytes, it's overallocated. One created from a list gives me a crash under ASAN; could you add a test with it?

bytes.fromhex(array.array('B', [ord('2')] * 5))

[StanFromIreland]

In the CI, we are running the Python test suite on Python built with Address Sanitizer. This build doesn't detect usage of uninitialized memory?

We only run with ASan and UBSan, however neither track uninitialised memory. We'd have to run with MSan.

Edit: I went off on a little tangent, but I wrote a patch to add MSan to the CI: #158625

[StanFromIreland]

This is dead logic now.

[vstinner]

Oh well spotted! I removed the dead code.

[StanFromIreland]

Also, I think we should add an entry, it's a bug fix after all.

[vstinner]

Apparently, when array.array() created from bytes, it's overallocated. One created from a list gives me a crash under ASAN

Oh, implementation details can be very surprising sometimes :-D

could you add a test with it?

Sure, I added two tests to test the two modified code paths, using an array created from a list.

I tested manually that the two tests are detected by Valgrind without the fix:

test_fromhex_ub (test.test_bytes.BytesTest.test_fromhex_ub) ... 

==2131686== Invalid read of size 1
==2131686==    at 0x497DA4: _PyBytes_FromHex (bytesobject.c:2705)
==2131686==    by 0x497E62: bytes_fromhex_impl (bytesobject.c:2635)
==2131686==    by 0x497EA7: bytes_fromhex (bytesobject.c.h:1261)
==2131686==    by 0x4F51FC: cfunction_vectorcall_O (methodobject.c:535)
==2131686==    by 0x49BFED: _PyObject_VectorcallTstate (pycore_call.h:149)
==2131686==    by 0x49C0B5: PyObject_Vectorcall (call.c:327)
==2131686==    by 0x5B1597: _Py_VectorCallInstrumentation_StackRefSteal (ceval.c:770)
==2131686==    by 0x5B8723: _PyEval_EvalFrameDefault (generated_cases.c.h:1906)
==2131686==    by 0x5D03B6: _PyEval_EvalFrame (pycore_ceval.h:122)
==2131686==    by 0x5D0581: _PyEval_Vector (ceval.c:2176)
==2131686==    by 0x49BCD9: _PyFunction_Vectorcall (call.c:413)
==2131686==    by 0x49BFED: _PyObject_VectorcallTstate (pycore_call.h:149)
==2131686==  Address 0x14630416 is 0 bytes after a block of size 6 alloc'd
==2131686==    at 0x4841AE6: malloc (vg_replace_malloc.c:447)
==2131686==    by 0x4FFD6A: _PyMem_RawMalloc (obmalloc.c:66)
==2131686==    by 0x514579: PyMem_Malloc (obmalloc.c:1257)
==2131686==    by 0x158A7FBB: newarrayobject_untracked (arraymodule.c:793)
==2131686==    by 0x158A9E49: array_new (arraymodule.c:3003)
==2131686==    by 0x5309ED: type_call (typeobject.c:2442)
==2131686==    by 0x49BE61: _PyObject_MakeTpCall (call.c:242)
==2131686==    by 0x49C067: _PyObject_VectorcallTstate (pycore_call.h:147)
==2131686==    by 0x49C0B5: PyObject_Vectorcall (call.c:327)
==2131686==    by 0x5B1597: _Py_VectorCallInstrumentation_StackRefSteal (ceval.c:770)
==2131686==    by 0x5B8723: _PyEval_EvalFrameDefault (generated_cases.c.h:1906)
==2131686==    by 0x5D03B6: _PyEval_EvalFrame (pycore_ceval.h:122)
==2131686== 
==2131686== Invalid read of size 1
==2131686==    at 0x497D5D: _PyBytes_FromHex (bytesobject.c:2717)
==2131686==    by 0x497E62: bytes_fromhex_impl (bytesobject.c:2635)
==2131686==    by 0x497EA7: bytes_fromhex (bytesobject.c.h:1261)
==2131686==    by 0x4F51FC: cfunction_vectorcall_O (methodobject.c:535)
==2131686==    by 0x49BFED: _PyObject_VectorcallTstate (pycore_call.h:149)
==2131686==    by 0x49C0B5: PyObject_Vectorcall (call.c:327)
==2131686==    by 0x5B1597: _Py_VectorCallInstrumentation_StackRefSteal (ceval.c:770)
==2131686==    by 0x5B8723: _PyEval_EvalFrameDefault (generated_cases.c.h:1906)
==2131686==    by 0x5D03B6: _PyEval_EvalFrame (pycore_ceval.h:122)
==2131686==    by 0x5D0581: _PyEval_Vector (ceval.c:2176)
==2131686==    by 0x49BCD9: _PyFunction_Vectorcall (call.c:413)
==2131686==    by 0x49BFED: _PyObject_VectorcallTstate (pycore_call.h:149)
==2131686==  Address 0x14728e35 is 0 bytes after a block of size 5 alloc'd
==2131686==    at 0x4841AE6: malloc (vg_replace_malloc.c:447)
==2131686==    by 0x4FFD6A: _PyMem_RawMalloc (obmalloc.c:66)
==2131686==    by 0x514579: PyMem_Malloc (obmalloc.c:1257)
==2131686==    by 0x158A7FBB: newarrayobject_untracked (arraymodule.c:793)
==2131686==    by 0x158A9E49: array_new (arraymodule.c:3003)
==2131686==    by 0x5309ED: type_call (typeobject.c:2442)
==2131686==    by 0x49BE61: _PyObject_MakeTpCall (call.c:242)
==2131686==    by 0x49C067: _PyObject_VectorcallTstate (pycore_call.h:147)
==2131686==    by 0x49C0B5: PyObject_Vectorcall (call.c:327)
==2131686==    by 0x5B1597: _Py_VectorCallInstrumentation_StackRefSteal (ceval.c:770)
==2131686==    by 0x5B8723: _PyEval_EvalFrameDefault (generated_cases.c.h:1906)
==2131686==    by 0x5D03B6: _PyEval_EvalFrame (pycore_ceval.h:122)
==2131686== 

[vstinner]

@StanFromIreland:

Also, I think we should add an entry, it's a bug fix after all.

Ok, I added a Changelog entry. I also removed dead code.

[StanFromIreland]

LGTM, just two little nits.

[miss-islington-app]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15.
🐍🍒⛏🤖

[bedevere-app]

GH-158691 is a backport of this pull request to the 3.15 branch.

[bedevere-app]

GH-158692 is a backport of this pull request to the 3.14 branch.

[vstinner]

Merged. Thanks for reviews!