gh-126554: ctypes: Correctly handle NULL dlsym values

[grgalex]

For dlsym(), a return value of NULL does not necessarily indicate an error [1].

Therefore, to avoid using stale (or NULL) dlerror() values, we must:

1. clear the previous error state by calling dlerror()
2. call dlsym()
3. call dlerror()

If the return value of dlerror() is not NULL, an error occured.

In our case of ctypes, I understand that we subjectively treat a NULL return value from dlsym() as an error, so we must format a special string for that.

[1] https://man7.org/linux/man-pages/man3/dlsym.3.html

• Issue: ctypes: We do not correctly handle NULL dlsym() return values #126554

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[efimov-mikhail]

On success, these functions return the address associated with
symbol. On failure, they return NULL; the cause of the error can
be diagnosed using dlerror(3).

This is direct quotation from the man page you linked.
Why you think this is correct?

dlsym(), a return value of NULL does not necessarily indicate an error [1].

[grgalex]

@efimov-mikhail

Indeed, I failed to mention that at some point the man page contradicts itself, and I believe that the part you quoted is something the maintainer forgot to remove.

[edit: added this part]

I say this because further up, in the DESCRIPTION, the man page states:

In unusual cases (see NOTES) the value of the symbol could
actually be NULL.  Therefore, a NULL return from dlsym() need not
indicate an error.  The correct way to distinguish an error from
a symbol whose value is NULL is to call dlerror(3) to clear any
old error conditions, then call dlsym(), and then call dlerror(3)
again, saving its return value into a variable, and check whether
this saved value is not NULL.

[end of edit]

Additionally, further down, in the NOTES segment, the man page states:

There are several scenarios when the address of a global symbol
is NULL.  For example, a symbol can be placed at zero address by
the linker, via a linker script or with --defsym command-line
option.  Undefined weak symbols also have NULL value.  Finally,
the symbol value may be the result of a GNU indirect function
(IFUNC) resolver function that returns NULL as the resolved
value.  In the latter case, dlsym() also returns NULL without
error.  However, in the former two cases, the behavior of GNU
dynamic linker is inconsistent: relocation processing succeeds
and the symbol can be observed to have NULL value, but dlsym()
fails and dlerror() indicates a lookup error.

[efimov-mikhail]

Understood. IMO, it would be better if we can make an actual reproducible test case, and put this case into regular testing procedure.

[grgalex]

See reproducer here: #126554 (comment)

[ZeroIntensity]

It's good to see new contributors :)

Unfortunately, this change does need a test. If it's really, really difficult to add one, then I guess we'll figure things out, but in general, it's difficult for us to accept even tiny bugfixes without proper tests.

For something like this, as long as the test works, it's probably OK. You could do something as hacky as hard-coding a byte string into a test file and unpacking that into a .so at runtime, and just disable all the systems that don't work. But it would be preferable if you could find a library that actually triggers this problem in the wild.

[grgalex]

@ZeroIntensity

1. I understand that I can just click "Commit" on your proposed changes, and then we can squash before merging
2. With some grepping I found out that we probably also need to change

   cpython/Modules/_ctypes/callproc.c

   Line 1626 in b19d12f

   ptr = dlsym((void*)handle, name);

   
   using the same approach.
3. Regarding the test case, I'm thinking something similar to https://github.com/python/cpython/blob/b19d12f44735583e339ee6ee560588fcf22633b4/Lib/test/test_ctypes/test_find.py, where we will:
   1. Write a fixed string (the content of foo.c from the reproducer),
   2. Compile (as does the test_find testcase) into a shared lib
   3. Load, and access the foo object.

[grgalex]

(2) is done, pushing a commit. I also added a comment that explicitly states why we call dlerror() before dlsym().

From a project maintaner's POV, if we have 3 instances of this phenomenon, do we add a comment disclaimer at every instance? (This is the approach I took, but I'm unsure what the best / most common practice is.)

[grgalex]

Proceeding with the test case (3), will let you know when ready.

[ZeroIntensity]

• I understand that I can just click "Commit" on your proposed changes, and then we can squash before merging

Yup. In fact, that's the preferred workflow for us. Force pushing onto one commit is very much discouraged.

2. With some grepping I found out that we probably also need to change

That looks like a problem too, yeah. Thanks!

I'll create a separate issue if I can come up with a repro for the locale issue. (I'll CC you on it, if you want to author that PR as well.)

[grgalex]

Added a commit with the test case under test_ctypes/test_dlerror. PTAL.

Running on the PR branch, it passes :)

On HEAD of main, it explodes (segfault).

Do we have any way of handling this gracefully (maybe running a subprocess with the interpreter?), or is it the correct way to go?

[efimov-mikhail]

Do we have any way of handling this gracefully (maybe running a subprocess with the interpreter?)

Could you please explain, what exactly are you speaking about?
Gracefully handling what?

[grgalex]

If we want to be pedantic, we must check all 3 code paths that invoke dlsym().

In this case, we only check one (PyCFuncPtr_FromDll).

On one hand, since our amendment is the same across all 3 funcs, we should theoretically be OK.

[grgalex]

Do we have any way of handling this gracefully (maybe running a subprocess with the interpreter?)

Could you please explain, what exactly are you speaking about? Gracefully handling what?

@efimov-mikhail

If I run the test on the main branch, the Python interpreter gets a segmentation fault.

What I'm saying is that in a scenario where someone runs all tests, when the ./python -m test process encounters this test, it will exit with a segmentation fault and terminate the testing procedure. It won't mark the test as failing. It will explode.

Maybe I have misunderstood something regarding the philosophy of testing.

[ZeroIntensity]

Run the test in a subprocess and check the return code.

[encukou]

Well, not all buildbots liked it :(

[ZeroIntensity]

I'm not sure if the ASan failures are related.

[picnixz]

One PEP-7 nitpick and two de-chop suggestions. I didn't write suggestions for the PyErr_SetString usages since I wasn't sure it fits on 80 chars.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit ab22349 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[ZeroIntensity]

Let's try the build bots again and see if it holds up now.

[ZeroIntensity]

Buildbots are all good, the one failure is a problem with the DRC API that got merged the other day, which is unrelated. I'll submit a (separate) patch to fix that. I think we're good to merge!

[miss-islington-app]

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

[miss-islington-app]

Sorry, @grgalex and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8717f792f7cc343599dc60daf80630cceb66145b 3.12

[bedevere-app]

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

[efimov-mikhail]

@grgalex, you've finished it! Congrats with your first contribution to CPython!

[grgalex]

Thanks for the guidance @efimov-mikhail, @ZeroIntensity, @encukou.

Looking forward to collaborating again in the future!

[picnixz]

3.13 backports were done but not 3.12; @encukou Do you want the 3.12 backport in the end or can we close the issue with only 3.13 and 3.14 being affected?

[encukou]

As I said, I'm fine not backporting. If anyone disagrees, open the PR :)

[bedevere-app]

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