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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f616fe
fix UBSan failure in `dictobject.c`
picnixz Dec 3, 2024
f164f24
use `dk_log2_index_bytes=3` in empty dicts
picnixz Dec 6, 2024
24b3f98
remove un-necessary cast
picnixz Dec 6, 2024
91defd4
Merge branch 'main' into fix/ubsan/dict/misaligned-loads-127563
picnixz Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 :)

}
else {
PyDictUnicodeEntry *ep = oldentries;
Expand Down
Loading