Skip to content
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

prevent misuse of PyTuple_Pack #1218

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Conversation

astrelsky
Copy link
Contributor

@astrelsky astrelsky commented Sep 16, 2024

This prevents simple mistakes such as the following

bases = PyTuple_Pack(1, &PyLong_Type, PyJPObject_Type);

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.21%. Comparing base (66f8c6c) to head (1b76a40).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
native/python/pyjp_package.cpp 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   87.21%   87.21%   -0.01%     
==========================================
  Files         113      113              
  Lines       10281    10277       -4     
  Branches     4065     4088      +23     
==========================================
- Hits         8967     8963       -4     
  Misses        718      718              
  Partials      596      596              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Thrameos
Copy link
Contributor

Minor suggestion.... given that all uses of this function should be placed in a JPPyObject wrapper via call to we don't leak it would seem like combining the two would make sense. It would take some minor reworking of a few sections which were already dangerous for exceptions.

@astrelsky
Copy link
Contributor Author

Minor suggestion.... given that all uses of this function should be placed in a JPPyObject wrapper via call to we don't leak it would seem like combining the two would make sense. It would take some minor reworking of a few sections which were already dangerous for exceptions.

Agreed. I'll do this tonight.

Comment on lines -407 to -408
PyObject *args = PyTuple_Pack(1, PyLong_FromLongLong(l));
return JPPyObject::call(PyLong_Type.tp_new((PyTypeObject*) wrapper.get(), args, nullptr));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was this and the two below a leak?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems likely. Once had leaks everwhere. Hence need to write the C++ wrapper for python API. Clearly this was missed.

@astrelsky
Copy link
Contributor Author

astrelsky commented Sep 16, 2024

The changes were mostly painless. I only had to think about one change which was in PyJPClass_FromSpecWithBases. I left a comment where it should not be used in that function.

Seems like the mac compiler is a bit over picky about constexpr. It's technically not wrong, but the standard is not that strict. It was only added out of habit to begin with. Not sure why it doesn't fail to compile anything using a standard c++ header 😂.

Comment on lines -407 to -408
PyObject *args = PyTuple_Pack(1, PyLong_FromLongLong(l));
return JPPyObject::call(PyLong_Type.tp_new((PyTypeObject*) wrapper.get(), args, nullptr));
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems likely. Once had leaks everwhere. Hence need to write the C++ wrapper for python API. Clearly this was missed.

@Thrameos
Copy link
Contributor

Merge for upcoming release.

@Thrameos Thrameos merged commit 09f325f into jpype-project:master Sep 26, 2024
25 of 26 checks passed
@astrelsky astrelsky deleted the tuple branch September 26, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants