libxml2 version check #299
Conversation
- Expose both the compiled version and the linked version of libxml2 - Do a check that the versions match when initializing the module. Otherwise raise an exception. This seems like a better result then a difficult to diagnose segfault.
jonathangreen
force-pushed
the
feature/version-check
branch
from
cca84ee
to
ee9fbd0
Compare
|
The check for lxml-loaded-libxml2 vs. xmlsec-loaded-libxml2 may prevent version mismatch problems, but I'm not sure it counts as a solution to #283 - my interpretation of that ticket is to actually make mixed versions work reliably. I think the |
|
Your criticism is valid; indeed, what we have here is more of a mitigation measure rather than a definitive solution. For #283, I thought having some checks in place was safer then no checks at all, so this PR met the criteria for resolving that issue.
I don't believe this is the case. In CI right now, you can see examples of both of these checks failing. The broken CI workflows are inadvertently providing a test case for this PR 😄 Without these checks in place the tests instead fail with Fatal Python Errors or segfaults. I think this is proof that the check does not duplicate a check already being done by Example of
An example of
In both cases, I think the results with this PR in place are preferable to the Python Errors that occur otherwise. They are more informative, and point more directly to the cause of the issue. I do agree with you though, it would be a better solution if the library mismatch were just seamlessly handled. I, unfortunately, don't have the expertise to implement a solution like that and I thought the checks in this PR provided a helpful mitigation. I am totally open to feedback on refining these checks or closing this PR in favor of a better solution to #283. |
But that test is installing So isn't the failure likely caused by a mismatch between |
|
@nosnilmot that is a good point. I pushed a new commit that removes the |
| if (version == NULL) { | ||
| goto FINALIZE; | ||
| } | ||
| if (!PyTuple_Check(version) || PyTuple_Size(version) != 3) { |
There was a problem hiding this comment.
It's probably enough to check "size < 2" here, if all you look at is the first two entries.
| PyObject* major = PyTuple_GetItem(version, 0); | ||
| PyObject* minor = PyTuple_GetItem(version, 1); | ||
|
|
||
| if (!PyLong_Check(major) || !PyLong_Check(minor)) { | ||
| goto FINALIZE; | ||
| } | ||
|
|
||
| if (PyLong_AsLong(major) != PyXmlSec_GetLibXmlVersionMajor() || PyLong_AsLong(minor) != PyXmlSec_GetLibXmlVersionMinor()) { | ||
| goto FINALIZE; | ||
| } |
There was a problem hiding this comment.
While this should usually work, users can easily assign tuples with large integers to etree.LIBXML_VERSION, which would then make PyLong_AsLong() fail here and the code continues with an exception set.
I suggest adding an exception check also to the PyTuple_GetItem() calls above, because why not.
There was a problem hiding this comment.
I thought that PyLong_Check handled this case, but re-reading the docs now I see that it doesn't. Thanks for the feedback here.
| FINALIZE: | ||
| // Cleanup our references, and return the result | ||
| Py_XDECREF(lxml); | ||
| Py_XDECREF(version); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
This should call PyErr_Clear(), in case we got here due to an exception.
|
@scoder Thank you for the review on this. I created a small follow up PR that incorporates your feedback (#300), since this one was already merged. If you have time, can you do a quick review of that PR as well and make sure I correctly captured your feedback? @jimjag once @scoder has a change to take a look at #300, it might be good to get that one merged as well before a release goes out. |
|
It's discouraged to call into the CPython C-API with a live exception set. This is not always enforced, but can lead to misbehaviour and crashes, and the behaviour may change in the future. You need to check for exceptions right after the call that may have raised it.
I think the code in this PR was mostly ok, with one additional check when unpacking the pylong, and with a general PyErr_Clear() in the end.
|
I believe this should resolve #283.
I've encountered ongoing issues with xmlsec builds on certain platforms using the latest code. While troubleshooting, I utilized #289, but it didn't consistently provide the necessary information.
Previously,
get_libxml_versionreturned the version oflibxml2thatxmlsecwas compiled against, whichlxmlrefers to asLIBXML_COMPILED_VERSION. I've introduced a new function calledget_libxml_compiled_versionto supply this information. Additionally, I've updatedget_libxml_versionto return the currently loaded version oflibxml2, similar toLIBXML_VERSIONfromlxml.To address #283, I added a new check that occurs when
xmlsecis imported:Verification of matching major and minor versions fromRemoved see libxml2 version check #299 (comment)get_libxml_compiled_versionandget_libxml_version.lxml.etree.LIBXML_VERSIONandget_libxml_version.If these checks fail, an
xmlsec.Erroris raised, providing relevant details. This should make it easier to troubleshoot when library versions are mismatched, rather then trying to trace down where a segfault came from.