Skip to content

gh-127563: fix UBSan failure in dictobject.c #127568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Dec 3, 2024

@@ -2012,7 +2012,7 @@ dictresize(PyInterpreterState *interp, PyDictObject *mp,
if (unicode) { // combined unicode -> combined unicode
PyDictUnicodeEntry *newentries = DK_UNICODE_ENTRIES(newkeys);
if (oldkeys->dk_nentries == numentries && mp->ma_keys->dk_kind == DICT_KEYS_UNICODE) {
memcpy(newentries, oldentries, numentries * sizeof(PyDictUnicodeEntry));
memcpy(newentries, (const void *)oldentries, numentries * sizeof(PyDictUnicodeEntry));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. The only change is the explicit cast to const void *?

Copy link
Member

@chris-eibl chris-eibl Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this would silence the UBSan misaligned warning, since by definition a void* can be read from any location.

But IMHO this is dangerous, because the UBSan warning given in the issue states that a PyDictUnicodeEntry * is going to be read from 0x5f7d064233d1, with an alignment of 8 (so this seems to stem from a 64 bit build).

Since this results from _DK_ENTRIES, this can only result from dk_log2_index_bytes=0 due to

size_t index = (size_t)1 << dk->dk_log2_index_bytes;

If I read the code correctly, this is only set via new_keys_object which has a protective

assert(log2_size >= PyDict_LOG_MINSIZE);

so that further down in the code dk_log2_index_bytes should never be smaller than PyDict_LOG_MINSIZE=3.

Hence, I assume this is not a debug build.

Luckily, new_keys_object has only four callers, and two of them pass constants (PyDict_LOG_MINSIZE and NEXT_LOG2_SHARED_KEYS_MAX_SIZE=6), this leaves dict_new_presized and dictresize, where the latter unfortunately has many callers :(

Maybe this helps and hopefully I am not completely mistaken ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Py_EMPTY_KEYS has dk_log2_index_bytes=0. But I don't know why it is passed to dictresize.
Can we get stacktrace for the UBSan error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • When we create an empty dict, we use Py_EMPTY_KEYS as the keys object
  • When we first insert into that empty dict, we resize it (via dictresize)
  • In that case oldkeys is Py_EMPTY_KEYS and oldentries is a misaligned PyDictUnicodeEntry *
  • memcpy is called with numentries=0 and an invalid/misaligned pointer for oldentries.
  • The UBSan message is confusing: there's not really any misaligned read (it's a zero-sized memcpy), but there's still undefined behavior
  • It's undefined behavior to call memcpy with an invalid pointer, even if count is zero.
  • It's also undefined behavior to create the misaligned pointer, even if we don't ever dereference. In other words, calling DK_UNICODE_ENTRIES() on Py_EMPTY_KEYS is UB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we first insert into that empty dict, we resize it (via dictresize)

insert_to_emptydict() is optimized for inserting into dict using Py_EMPTY_KEYS.
stacktrace can help to find where insert_to_emptydict() can be used.

Anyway, dk_log2_index_bytes in Py_EMPTY_DICT can be 3 instead of 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be pesky, I knew this from C++

But for https://en.cppreference.com/w/c/language/pointer I do not see this or anything similar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the C11 standard draft, 6.5.6 bullet points 7 and 8. There are probably other references as well.

Copy link
Member Author

@picnixz picnixz Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is documented in Section 6.5.9.7 from the C20 (emphasis mine):

Two pointers compare equal if and only if [...], both are pointers to one past the last element of the same array object, or one is a pointer to one past the end of one array object and the other is a pointer to the start of a different array object that happens to immediately follow the first array object in the address space.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also some examples in https://en.cppreference.com/w/c/language/struct involving structs with flexible array members. The examples are probably not authoritative, but I tend to trust the site:

struct s { int n; double d[]; }; // s.d is a flexible array member
double *dp;
...
s2 = malloc(sizeof (struct s) + 6);  // same, but UB to access because 2 bytes are
                                     // missing to complete 1 double
dp = &(s2->d[0]);            // OK, can take address just fine
*dp = 42;                    // undefined behavior

Note that constructing dp is okay even if evaluating *dp is UB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - flexible arrays to the rescue. Luckily, we have it here in _dictkeysobject, which is internal, since in C++ they are not (yet) allowed :)

@picnixz picnixz marked this pull request as draft December 5, 2024 14:07
@picnixz picnixz marked this pull request as ready for review December 6, 2024 11:20
@picnixz picnixz marked this pull request as draft December 6, 2024 11:21
@picnixz picnixz force-pushed the fix/ubsan/dict/misaligned-loads-127563 branch from 3a3fef4 to f164f24 Compare December 6, 2024 11:22
@picnixz picnixz marked this pull request as ready for review December 6, 2024 11:24
@picnixz
Copy link
Member Author

picnixz commented Dec 6, 2024

I locally tested that Clang is happy by adding __attribute__((no_sanitize("alignment")) on find_first_nonascii just to check if the dict's UBSan disappeared accordingly.

@picnixz picnixz requested a review from colesbury December 6, 2024 13:37
@methane
Copy link
Member

methane commented Dec 7, 2024

How about this?

$ git diff
diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index a13d8084d14..04293af210e 100644
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -1988,6 +1988,9 @@ dictresize(PyInterpreterState *interp, PyDictObject *mp,
             free_values(oldvalues, IS_DICT_SHARED(mp));
         }
     }
+    else if (oldkeys == Py_EMPTY_KEYS) {
+        set_keys(mp, newkeys);
+    }
     else {  // oldkeys is combined.
         if (oldkeys->dk_kind == DICT_KEYS_GENERAL) {
             // generic -> generic
@@ -2041,14 +2044,12 @@ dictresize(PyInterpreterState *interp, PyDictObject *mp,

         set_keys(mp, newkeys);

-        if (oldkeys != Py_EMPTY_KEYS) {
 #ifdef Py_REF_DEBUG
-            _Py_DecRefTotal(_PyThreadState_GET());
+        _Py_DecRefTotal(_PyThreadState_GET());
 #endif
-            assert(oldkeys->dk_kind != DICT_KEYS_SPLIT);
-            assert(oldkeys->dk_refcnt == 1);
-            free_keys_object(oldkeys, IS_DICT_SHARED(mp));
-        }
+        assert(oldkeys->dk_kind != DICT_KEYS_SPLIT);
+        assert(oldkeys->dk_refcnt == 1);
+        free_keys_object(oldkeys, IS_DICT_SHARED(mp));
     }

     STORE_KEYS_USABLE(mp->ma_keys, mp->ma_keys->dk_usable - numentries);

@encukou
Copy link
Member

encukou commented Dec 10, 2024

I'll merge to fix the UB reports.
Even if we rearrange dictresize later, dk_log2_index_bytes should be 3.

@encukou encukou merged commit 9af96f4 into python:main Dec 10, 2024
39 checks passed
@miss-islington-app
Copy link

Thanks @picnixz for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 10, 2024
…-127568)

This fixes a UBSan failure (unaligned zero-size memcpy) in `dictobject.c`.
(cherry picked from commit 9af96f4)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 10, 2024
…-127568)

This fixes a UBSan failure (unaligned zero-size memcpy) in `dictobject.c`.
(cherry picked from commit 9af96f4)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Dec 10, 2024

GH-127798 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 10, 2024
encukou pushed a commit that referenced this pull request Dec 11, 2024
…) (GH-127798)

This fixes a UBSan failure (unaligned zero-size memcpy) in `dictobject.c`.
(cherry picked from commit 9af96f4)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@encukou encukou added needs backport to 3.12 only security fixes and removed needs backport to 3.12 only security fixes labels Dec 11, 2024
@miss-islington-app
Copy link

Thanks @picnixz for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 11, 2024
…-127568)

This fixes a UBSan failure (unaligned zero-size memcpy) in `dictobject.c`.
(cherry picked from commit 9af96f4)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Dec 11, 2024

GH-127813 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 11, 2024
encukou pushed a commit that referenced this pull request Dec 12, 2024
…) (GH-127813)

This fixes a UBSan failure (unaligned zero-size memcpy) in `dictobject.c`.
(cherry picked from commit 9af96f4)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz picnixz deleted the fix/ubsan/dict/misaligned-loads-127563 branch December 13, 2024 17:08
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
…-127568)

This fixes a UBSan failure (unaligned zero-size memcpy) in `dictobject.c`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants