libxml2 version check by jonathangreen · Pull Request #299 · xmlsec/python-xmlsec · GitHub

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_version returned the version of libxml2 that xmlsec was compiled against, which lxml refers to as LIBXML_COMPILED_VERSION. I've introduced a new function called get_libxml_compiled_version to supply this information. Additionally, I've updated get_libxml_version to return the currently loaded version of libxml2, similar to LIBXML_VERSION from lxml.

To address #283, I added a new check that occurs when xmlsec is imported:

Verification of matching major and minor versions from get_libxml_compiled_version and get_libxml_version. Removed see libxml2 version check #299 (comment)
Verification of matching major and minor versions from lxml.etree.LIBXML_VERSION and get_libxml_version.

If these checks fail, an xmlsec.Error is 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.

- 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.

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 PyXmlSec_CheckLibXmlLibraryVersion() check for compile vs. runtime version of libxml2 is overly restrictive and unnecessary, regular ABI compatibility for compile vs. runtime is already handled by libxml2 soname. Are there really any reported segfaults from this mismatch rather than the above?

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 think the PyXmlSec_CheckLibXmlLibraryVersion() check for compile vs. runtime version of libxml2 is overly restrictive and unnecessary, regular ABI compatibility for compile vs. runtime is already handled by libxml2 soname.

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 libxml2.

Example of PyXmlSec_CheckLibXmlLibraryVersion failing is in the macosx workflow:

Example from master without the check:
- Results in: Fatal Python error: Floating point exception
Example from ci run for this PR
- Results in: xmlsec.InternalError: (-1, 'xmlsec libxml2 library compiled version vs runtime version mismatch')

An example of PyXmlSec_CheckLxmlLibraryVersionfailing is the sdist workflow:

Example from master without the check:
- Results in: Fatal Python error: Aborted
Example from ci run for this PR
- Results in: xmlsec.InternalError: (-1, 'xmlsec.InternalError: (-1, 'lxml & xmlsec libxml2 library version mismatch')

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.

Example of PyXmlSec_CheckLibXmlLibraryVersion failing is in the macosx workflow:

Example from master without the check:

Results in: Fatal Python error: Floating point exception

Example from ci run for this PR

Results in: xmlsec.InternalError: (-1, 'xmlsec libxml2 library compiled version vs runtime version mismatch')

But that test is installing lxml from a wheel:

Collecting lxml!=4.7.0,>=3.8.0 (from -r requirements.txt (line 1))
  Using cached lxml-5.1.0-cp37-cp37m-macosx_10_9_x86_64.whl.metadata (3.5 kB)

So isn't the failure likely caused by a mismatch between libxml2 version used by lxml vs xmlsec rather than the mismatch between compile-time and run-time version for xmlsec? i.e. it would be caught by the PyXmlSec_CheckLxmlLibraryVersion check too.

@nosnilmot that is a good point. I pushed a new commit that removes the PyXmlSec_CheckLibXmlLibraryVersion check you mentioned and you are right, the second check catches the CI issue. Thanks for the great feedback on this one.

Looks good, thanks. This should make it much less likely that users run into crashing surprises.

It's probably enough to check "size < 2" here, if all you look at is the first two entries.

I hadn't looked at the exception handling part.

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.

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.

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.

Version check when setting up lxml

ee9fbd0

- 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 March 22, 2024 16:33

jonathangreen mentioned this pull request Mar 22, 2024

Release the new version #284

Closed

Remove PyXmlSec_CheckLibXmlLibraryVersion check

497971c

Add type hints

c7e3208

jimjag merged commit 1525fe0 into xmlsec:master Mar 27, 2024

scoder reviewed Mar 27, 2024

View reviewed changes

jonathangreen mentioned this pull request Mar 27, 2024

Update to libxml2 version incorporating code review feedback #300

Merged

jonathangreen deleted the feature/version-check branch March 27, 2024 12:50

jonathangreen mentioned this pull request Mar 28, 2024

Update osx workflow #296

Merged

+                  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;
+                  }

+              FINALIZE:
+                  // Cleanup our references, and return the result
+                  Py_XDECREF(lxml);
+                  Py_XDECREF(version);
+                  return result;
+              }

libxml2 version check by jonathangreen · Pull Request #299 · xmlsec/python-xmlsec · GitHub

Uh oh!

libxml2 version check - #299

libxml2 version check#299
jimjag merged 3 commits into
xmlsec:masterfrom
jonathangreen:feature/version-check

jonathangreen commented Mar 22, 2024 •
edited
Loading

Uh oh!

Uh oh!

nosnilmot commented Mar 24, 2024

Uh oh!

jonathangreen commented Mar 24, 2024

Uh oh!

nosnilmot commented Mar 24, 2024

Uh oh!

jonathangreen commented Mar 25, 2024

Uh oh!

scoder left a comment

Uh oh!

scoder Mar 27, 2024

Uh oh!

scoder left a comment

Uh oh!

scoder Mar 27, 2024

Uh oh!

jonathangreen Mar 27, 2024

Uh oh!

scoder Mar 27, 2024

Uh oh!

jonathangreen commented Mar 27, 2024

Uh oh!

scoder commented Mar 27, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

+                  if (version == NULL) {
+                      goto FINALIZE;
+                  }
+                  if (!PyTuple_Check(version) || PyTuple_Size(version) != 3) {

libxml2 version check by jonathangreen · Pull Request #299 · xmlsec/python-xmlsec · GitHub

Uh oh!

libxml2 version check - #299

libxml2 version check#299jimjag merged 3 commits intoxmlsec:masterxmlsec/python-xmlsec:masterfrom jonathangreen:feature/version-checkjonathangreen/python-xmlsec:feature/version-checkCopy head branch name to clipboard

Conversation

jonathangreen commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosnilmot commented Mar 24, 2024

Uh oh!

jonathangreen commented Mar 24, 2024

Uh oh!

nosnilmot commented Mar 24, 2024

Uh oh!

jonathangreen commented Mar 25, 2024

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

jonathangreen Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

scoder Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

jonathangreen commented Mar 27, 2024

Uh oh!

scoder commented Mar 27, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

libxml2 version check#299
jimjag merged 3 commits into
xmlsec:masterfrom
jonathangreen:feature/version-check

jonathangreen commented Mar 22, 2024 •
edited
Loading