Skip to content

Fixed inconsistencies generated by _md_shrink.#1229

Merged
asvetlov merged 1 commit intoaio-libs:masterfrom
Romain-Geissler-1A:fix-md_shrink
Jun 30, 2025
Merged

Fixed inconsistencies generated by _md_shrink.#1229
asvetlov merged 1 commit intoaio-libs:masterfrom
Romain-Geissler-1A:fix-md_shrink

Conversation

@Romain-Geissler-1A
Copy link
Contributor

This is causing assertion failures in ASSERT_CONSISTENT if an entry used to have a non null identity. Since the new entries will be concentrated at the beginning of the array, we shall "erase" the old entries that are at the end of the array and that won't be used anymore.

This is enough to fix the core dump issues found in the testsuite of httpie in Fedora Rawhide (see #1195 (comment))

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 29, 2025

CodSpeed Performance Report

Merging #1229 will not alter performance

Comparing Romain-Geissler-1A:fix-md_shrink (815ddff) with master (8ea6942)

Summary

✅ 245 untouched benchmarks

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jun 29, 2025
@asvetlov asvetlov self-assigned this Jun 30, 2025
@asvetlov
Copy link
Member

@Romain-Geissler-1A could you confirm that the error was raised by ASSERT_CONSISTENT() call?
I see neither a test nor a reproducer.
If it is an assertion in self-consistency check, I would suggest another fix: memset() the tail of the entries array with zeroes. It emulates the behavior of _md_resize() without requiring new memory allocation.

@asvetlov asvetlov requested a review from bdraco June 30, 2025 09:24
@Romain-Geissler-1A
Copy link
Contributor Author

Romain-Geissler-1A commented Jun 30, 2025

Hi,

Yes, in my case, the assert failure was exactly this one:

CHECK(entry->hash != -1);
and came from this call site:
ASSERT_CONSISTENT(md, false);
(there is a second call to ASSERT_CONSISTENT after the call to md_post_update in multidict.c as well. I investigate all this with gdb.

Despite my effort, I was unable to reproduce this reliably any other way then building the rpm with fedora packaging tool, installing a local httpie from the github sources, creating a virtual environment from the httpie Makefile, then removing the installation of multidict (which by default is pure python) and replace it by the one from the system using the previously built RPM. I also tried to compile a local multidict shared object manually but then for some reason I don't get, it was working (maybe assertions are disabled in local builds, and for some reason in Fedora it isn't, point that I already raised to one of my Fedora contact point). When trying to debug calls to multidict in httpie, I noticed that the fact of adding some print() calls has an influence on the reproducer (with enough traces, it works...).

I can try your suggestion of zeroing out every trailing entries.

@Romain-Geissler-1A
Copy link
Contributor Author

I pushed the new suggested implementation based on memset in this Fedora COPR https://copr.fedorainfracloud.org/coprs/romaingeissler1a/python-multidict/builds/:

The last remaining failure in httpie (tests/test_cli_utils.py::test_lazy_choices_help) is a known issue of the Python 3.14 upgrade, tracked in https://bugzilla.redhat.com/show_bug.cgi?id=2350335 With a stock multidict 6.6.2 RPM, some multi-header tests of httpie used to crash entirely, and abort the whole testing.

This is causing assertion failures in ASSERT_CONSISTENT if an entry
used to have a non null identity. Since the new entries will be
concentrated at the beginning of the array, we shall "erase" the old
entries that are at the end of the array and that won't be used anymore.

This is enough to fix the core dump issues found in the testsuite of
httpie in Fedora Rawhide.
@Romain-Geissler-1A Romain-Geissler-1A changed the title Fix inconsistencies generated by _md_shrink. Fixed inconsistencies generated by _md_shrink. Jun 30, 2025
@asvetlov
Copy link
Member

@Romain-Geissler-1A, please confirm that the assertion failure appears in debug mode only (MULTIDICT_DEBUG_BUILD=1).
ASSERT_CONSISTENT() is designed to do nothing in the regular build

@Romain-Geissler-1A
Copy link
Contributor Author

That's one of the issue here: in the Fedora case, "normal" builds are built with assertion enabled, while it seems it's not your intention. You can see the complete build log of the python-multidict package in Fedora here: https://download.copr.fedorainfracloud.org/results/romaingeissler1a/python-multidict/fedora-rawhide-aarch64/09227274-python-multidict/builder-live.log.gz In particular, we can see that multidict.c is compiled and linked like this:

  gcc -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fPIC -I/usr/include/python3.14 -c multidict/_multidict.c -o build/temp.linux-aarch64-cpython-314/multidict/_multidict.o -O3 -std=c11 -Wall -Wsign-compare -Wconversion -fno-strict-aliasing -Wno-conversion -Werror
  gcc -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,pack-relative-relocs -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-hardened-ld-errors -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -Wl,--build-id=sha1 -specs=/usr/lib/rpm/redhat/redhat-package-notes -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer build/temp.linux-aarch64-cpython-314/multidict/_multidict.o -L/usr/lib64 -o build/lib.linux-aarch64-cpython-314/multidict/_multidict.cpython-314-aarch64-linux-gnu.so

So the CFLAGS found in setup.py are ignored (in fedora setup.py is normally ignored, the pyproject.toml file is usually favored). Anyway, even we if we used the CFLAGS from setup.py, it's defined to be -O3 in normal release builds, which doesn't define -DNDEBUG by default (at least on Fedora). So assertions ARE enabled by default in Release builds in Fedora in the end. I have started an internal discussion about the default definition of -DNDEBUG internally with one of my Red Hat contact.

@Romain-Geissler-1A
Copy link
Contributor Author

Technically, if you run an OCI container 'quay.io/fedora/fedora:rawhide', and run this inside it:

bash-5.2# dnf install -y gdb https://download.copr.fedorainfracloud.org/results/romaingeissler1a/python-multidict/fedora-rawhide-x86_64/09227274-python-multidict/python3-multidict-6.6.2-1.fc43.x86_64.rpm https://download.copr.f
edorainfracloud.org/results/romaingeissler1a/python-multidict/fedora-rawhide-x86_64/09227274-python-multidict/python3-multidict-debuginfo-6.6.2-1.fc43.x86_64.rpm             
Updating and loading repositories:
 Fedora rawhide openh264 (From Cisco) - x86_64                                                                                                                                             100% |   5.9 KiB/s |   5.8 KiB |  00m01s
 Fedora - Rawhide - Developmental packages for the next Fedora release                                                                                                                     100% |   9.8 MiB/s |  21.8 MiB |  00m02s
Repositories loaded.
 https://download.copr.fedorainfracloud.org/results/romaingeissler1a/python-multidict/fedora-rawhide-x86_64/09227274-python-multidict/python3-multidict-debuginfo-6.6.2-1.fc43.x86_64.rpm  100% | 325.5 KiB/s | 183.9 KiB |  00m01s
 https://download.copr.fedorainfracloud.org/results/romaingeissler1a/python-multidict/fedora-rawhide-x86_64/09227274-python-multidict/python3-multidict-6.6.2-1.fc43.x86_64.rpm            100% | 122.7 KiB/s |  93.8 KiB |  00m01s
 (output skipped...)

bash-5.2# gdb /usr/lib64/python3.14/site-packages/multidict/_multidict.cpython-314-x86_64-linux-gnu.so
GNU gdb (Fedora Linux) 16.3-3.fc43
Copyright (C) 2024 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/lib64/python3.14/site-packages/multidict/_multidict.cpython-314-x86_64-linux-gnu.so...
Reading symbols from /usr/lib/debug/usr/lib64/python3.14/site-packages/multidict/_multidict.cpython-314-x86_64-linux-gnu.so-6.6.2-1.fc43.x86_64.debug...
(gdb) disas _md_check_consistency
Dump of assembler code for function _md_check_consistency:
Address range 0x2220 to 0x24d7: 
   0x0000000000002220 <+0>:     push   %rbp
   0x0000000000002221 <+1>:     mov    %rsp,%rbp
   0x0000000000002224 <+4>:     push   %r13
   0x0000000000002226 <+6>:     push   %r12

you can see that the symbol _md_check_consistency is defined, so that NDEBUG was not defined at compilation time and thus assertions are enabled.

@asvetlov
Copy link
Member

@Romain-Geissler-1A
Thanks for the explanation. I'll make a new release right now.

I wonder why Fedora doesn't respect CFLAGS from setup.py. It's ok that Fedora prefers pyproject.toml, the library provides it. In turn, pyproject.toml is configured to get additional metadata from setup.cfg and setup.py; this is a pretty standard approach.

I would say that in a debug build, the library performs a self-check after every multidict operation; it sacrifices performance, but is acceptable for debugging purposes. This is not what distros want to have.

@asvetlov asvetlov merged commit ac62105 into aio-libs:master Jun 30, 2025
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants