-
Notifications
You must be signed in to change notification settings - Fork 901
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
Raise import error on module import failure to avoid segfault #786
Raise import error on module import failure to avoid segfault #786
Conversation
It looks like @overstre hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here. Once you've signed reply with Appreciation of efforts, clabot |
[clabot:check] |
@confluentinc It looks like @overstre just signed our Contributor License Agreement. 👍 Always at your service, clabot |
What are your thoughts on removing the enum34 dependency all together. I'm actually not a huge fan of this dependency and we can easily achieve the desired functionality with constants as we do with logical offsets. |
I'm in favor of reducing dependencies and keeping the style consistent. |
@@ -1959,6 +1959,13 @@ PyObject *cfl_PyObject_lookup (const char *modulename, const char *typename) { | |||
return NULL; | |||
} | |||
|
|||
if (!module) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this, but I think the initial intent was for the if-statement above to handle this, could you change that one instead to check for module rather than modulename, and remove the first %s and modulename from the argument list.
Also, change it to PyExc_ImportError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I've updated the code.
brian@dev-brian-1:~/code/confluent-kafka-python$ python ~/list.py
Traceback (most recent call last):
File "/home/brian/list.py", line 5, in <module>
print producer.list_topics(topic=topic).topics.values()
ImportError: Module not found when looking up confluent_kafka.admin.ClusterMetadata
91e8618
to
22682e0
Compare
22682e0
to
11dd8e5
Compare
A dependency of confluent-kafka,
enum34
, published a broken version 1.1.8 that caused a segfault inconfluent-kafka
python library when accessing the moduleconfluent_kafka.admin
. For example, the list_topics call will try to import this module in functioncfl_PyObject_lookup
.This results in
module
pointer being nil in the return of this call:PyObject *module = PyImport_ImportModule(modulename);
and then it gets accessed here
obj = PyObject_GetAttrString(module, typename);
which results in a segfault:
This pull request is to check the value of module to ensure that it's not null otherwise return a runtime exception. This will make debugging this issue in the future.
Without patch
with patch: