itstool: 2.0.2 -> 2.0.6 #72342

chkno · 2019-10-31T01:14:49Z

Motivation for this change

To get python3 support. #63174 flipped itstool to python3, but itstool doesn't support python3 until 2.0.3 (and perhaps does not support it well until 2.0.5).

Pressing forward instead of rolling back at worldofpeace's suggestion in #63174 and #72335, where it is mentioned that other distros seem to be able to ship recent versions of itstool.

Tensions in this space seem two-fold. One set centers around libxml2 being a low-level C library with sharp edges, manual memory management, and performance concerns; the python libxml2 wrapper being quite thin (the most dubious character in this drama); and python's sentiment that it ought to be quite hard to crash the interpreter casually. This comes to a head in https://gitlab.gnome.org/GNOME/libxml2/issues/12 , where a use-after-free problem in idiomatic-looking python code is declared working-as-designed.

The other set is around python3 being more UTF-8-aware than libxml2's python wrapper, such as https://bugzilla.gnome.org/show_bug.cgi?id=789714 and https://src.fedoraproject.org/rpms/libxml2/blob/master/f/libxml2-2.9.8-python3-unicode-errors.patch

itstool is caught in this crossfire merely for being a widely-used python program that uses XML.

Things done

Notify maintainers

cc @jtojnar @worldofpeace @FRidh

To get python3 support. NixOS#63174 flipped itstool to python3, but itstool doesn't support python3 until 2.0.3 (and perhaps does not support it well until 2.0.5). Pressing forward instead of rolling back at worldofpeace's suggestion, who mentions that other distros seem to be able to ship recent versions of itstool. Tensions in this space seem two-fold. One set centers around libxml2 being a low-level C library with sharp edges, manual memory management, and performance concerns; the python libxml2 wrapper being quite thin (the most dubious character in this drama); and python's sentiment that it ought to be quite hard to crash the interpreter casually. This comes to a head in https://gitlab.gnome.org/GNOME/libxml2/issues/12 , where a use-after-free problem in idiomatic-looking python code is declared working-as-designed. The other set is around python3 being more UTF-8-aware than libxml2's python wrapper, such as https://bugzilla.gnome.org/show_bug.cgi?id=789714 and https://src.fedoraproject.org/rpms/libxml2/blob/master/f/libxml2-2.9.8-python3-unicode-errors.patch itstool is caught in this crossfire merely for being a widely-used python program that uses XML.

chkno · 2019-10-31T01:18:34Z

If this goes forward, #72335 (revert to python2) should not. I don't know which is all-around best. My interest here is just to be able to run NixOS tests on staging.

jtojnar · 2019-10-31T01:27:07Z

We will need to apply the opensuse patch/hack for this to work.

chkno · 2019-10-31T07:37:03Z

Maybe. I was able to reproduce the problem building gnome3.gnome_desktop fd9cfc4 describes with itstool 2.0.4, but I was not able to reproduce it with itstool 2.0.6. I.e., if 2.0.6 had been available when fd9cfc4 was committed, bumping to 2.0.6 (and python3) instead of reverting to 2.0.2 would have fixed the gnome3.gnome_desktop build problem just as well.

chkno · 2019-10-31T07:41:43Z

fd9cfc4 mentions that other packages were broken as well, but doesn't say which ones. #41616 , which that fix is attached to, is about the nixos.tests.gnome3 test.

I cannot build the gnome3 NixOS test with this PR. I get this error while trying to build gnome3.zenity, one of its dependencies:

Traceback (most recent call last):
  File "/nix/store/0ri31bhv3n57pxqaky3s8sqz72yqay7s-itstool-2.0.6/bin/itstool", line 27, in <module>
    import libxml2
  File "/nix/store/gf8s6kl3v1fgqaz52qd2ffnvja9s8rjv-libxml2+py-2.9.9/lib/python2.7/site-packages/libxml2.py", line 1, in <module>
    import libxml2mod
ImportError: dynamic module does not define module export function (PyInit_libxml2mod)
make[2]: *** [Makefile:554: bg/bg.stamp] Error 1
make[2]: Leaving directory '/build/zenity-3.32.0/help'
make[1]: *** [Makefile:424: all-recursive] Error 1
make[1]: Leaving directory '/build/zenity-3.32.0'
make: *** [Makefile:365: all] Error 2
builder for '/nix/store/nixp439kx1acw8s0nxbbssky118fhc8b-zenity-3.32.0.drv' failed with exit code 2
error: build of '/nix/store/nixp439kx1acw8s0nxbbssky118fhc8b-zenity-3.32.0.drv' failed

I can build zenity with the itstool-back-to-python2 PR #72335

FRidh · 2019-10-31T07:51:39Z

i have the impression Python 2 and 3 get mixed up in that trace. It's using libxml2Python which is Python2.

@chkno Could you push a commit with the patch here as well?

chkno · 2019-10-31T08:53:09Z

No.

I haven't seen any harm come of the problem described in https://bugzilla.opensuse.org/show_bug.cgi?id=1065270 other than the test case posted to that bug. The issue that caused nixpkgs to revert itstool from 2.0.4 to 2.0.2 is resolved just as well by bumping to 2.0.6, which is a much cleaner fix than patching a different, much more widely used library to resolve a problem in itstool. (edit: gnumeric is broken too)
No attempt by any party has been made to upstream that patch. That openSUSE bug was incorrectly marked a duplicate of another, only slightly related bug, which probably forestalled any attempt to push it upstream from there.
https://bugzilla.gnome.org/show_bug.cgi?id=789714 notes that there are several other UTF-8 problems not addressed in that patch.
Most of all: That patch calls PyErr_Clear() in an error-handling channel. That seems potentially problematic to me. I don't know this code well enough to evaluate the comment claiming that this is a benign thing to do, and digging in to find out would overflow my yak-shaving stack.

This change in pkgs/development/libraries/libxml2/default.nix applies that patch, but I don't feel comfortable authoring and proposing a git commit that does this:

+  patches = [ (fetchurl {
+    url = "https://bugzilla.opensuse.org/attachment.cgi?id=746044";
+    sha256 = "37eb81a8ec6929eed1514e891bff2dd05b450bcf0c712153880c485b7366c17c";
+  }) ];

(edit: c0cecd0 below proposes this patch without the PyErr_Clear() and with an explanation of why a band-aid is appropriate rather than trying for a proper fix.)

chkno · 2019-10-31T08:53:22Z

The zenity build failure is not related to itstool. Zenity builds just fine when the itstool 2.0.6 bump is applied immediately after the 2.0.4 -> 2.0.2 revert. I think this command will find the commit that introduced the zenity build failure:

git bisect start 6b7f343121b3d77a1c17532c900f8aa40ad8c5e2 4b649a99d8461c980e7028a693387dc48033c1f7
git bisect run bash -c 'git log -n1 -p 0c999c7521ee52dd00566643f3600627783d2550 | patch -p1 && nix-build -A gnome3.zenity -I nixpkgs=.; status=$?; git checkout pkgs/development/tools/misc/itstool/default.nix; exit $status'

jtojnar · 2019-10-31T08:58:49Z

@GrahamcOfBorg build gnome3.gnome-devel-docs gnumeric

FRidh · 2019-10-31T10:28:48Z

ok, the zenity build failure is because it uses gnome-doc-utils which is a Python 2 application, which depends on libxml2Python and propagates it. This ends up in zenity as a build time dependency, causing a mixture of Python 2 and 3 libraries.

Python libraries and especially applications ideally should not propagate their dependencies, but we're not there yet. We should be able to work around this by adding libxml2Python to pythonPath instead of propagatedBuildInputs.

jtojnar · 2019-10-31T10:51:38Z

Could we do the same thing meson does for now?

nixpkgs/pkgs/development/tools/build-managers/meson/default.nix

Line 31 in 46911eb

rm $out/nix-support/propagated-build-inputs

jtojnar · 2019-10-31T14:28:51Z

With gnumeric, I am still getting the segfault on your branch.

… crash 1. Gnumeric has unbalanced XML tags in its doc translations. 2. itstool's XML error handler tries to print this error with context. 3. libxml2's context snipper treats the data as bytes, not UTF-8. 4. python3Packages.libxml2 casts the context to a UTF-8 Python string. 5. itstool dereferences a null pointer. This patch intervenes at NixOS#4. In https://bugzilla.gnome.org/show_bug.cgi?id=789714#c4 , upstream suggests that intervening at NixOS#3 would be better -- that each of the four copies of xmlParserPrintFileContextInternal() have four additional UTF-8 problems, one of which is that the caret indicator ought to count "unicode characters" not bytes. But to position a caret correctly, a character count is not sufficient -- this would need to use icu's BiDi logic (with fallback to doing something wrong when libxml2 is configured not to use icu) -- which makes a 'correct' fix a much larger project than this simple band-aid.

chkno · 2019-11-01T00:34:44Z

(The bisect finished: a2c62e2 introduced the zenity build break.)

gnumeric -- thanks! Yes, that one is broken. And that helped me find more upstream issues tracking this: itstool/itstool#22 and https://gitlab.gnome.org/GNOME/libxml2/issues/64

I captured a valgrind --tool=memcheck trace of this problem:

Process terminating with default action of signal 11 (SIGSEGV)
 Access not within mapped region at address 0x0
   at 0x48B00F4: function_code_fastcall (in /nix/store/w0rck0g2i12c7zmmcmngfpnszc5238ry-python3-3.7.4/lib/libpython3.7m.so.1.0)
   by 0x48E2116: _PyFunction_FastCallDict (in /nix/store/w0rck0g2i12c7zmmcmngfpnszc5238ry-python3-3.7.4/lib/libpython3.7m.so.1.0)
   by 0xD8D438D: libxml_xmlErrorFuncHandler (in /nix/store/p8i1sm5x9bx6967f59fjb63rlm28iajh-libxml2-2.9.9-py/lib/python3.7/site-packages/libxml2mod.so)
   by 0xD948214: xmlParserPrintFileContextInternal (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD94857B: xmlReportError (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD94A06C: __xmlRaiseError (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD94E6C4: xmlFatalErrMsgStrIntStr (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD95C2AB: xmlParseEndTag2 (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960DA5: xmlParseElement (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960445: xmlParseContent (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960D42: xmlParseElement (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
   by 0xD960445: xmlParseContent (in /nix/store/kyi0j76yy6pnpz55cysx95a1lffp1wlz-libxml2-2.9.9/lib/libxml2.so.2.9.9)
...

The PyErr_Clear() is not needed to get Gnumeric to build, so I left it out.

I also created a gnumeric PR to fix the malformed XML that's causing these errors that itstool / python3Packages.libxml2 fails to handle gracefully.

That breaks packages that rely on Python like itstool does. NixOS#72342 (comment)

jtojnar · 2019-11-01T00:42:18Z

a2c62e2 looks unlikely to cause that. I think it is caused by #63147 as well.

Anyway, I confirmed that https://github.com/jtojnar/nixpkgs/tree/zenity-fix fixes that instance.

I would say that if you cherry-pick that and add the GNumeric patch we can merge this. We can always deal with the remaining packages later.

jtojnar · 2019-11-01T00:45:22Z

Or since you added the libxml hack, the GNumeric one is not necessary.

jtojnar

Or let's just merge this and I will handle the zenity fix separately.

jtojnar · 2019-11-01T01:26:18Z

Thanks.

That breaks packages that rely on Python like itstool does. #72342 (comment)

chkno mentioned this pull request Oct 31, 2019

itstool: Don't use Python 3 yet #72335

Closed

10 tasks

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000 labels Oct 31, 2019

jtojnar added a commit to jtojnar/nixpkgs that referenced this pull request Nov 1, 2019

gnome-doc-utils: do not propagate Python

90a364d

That breaks packages that rely on Python like itstool does. NixOS#72342 (comment)

ofborg bot added the 10.rebuild-darwin-stdenv label Nov 1, 2019

ofborg bot requested a review from edolstra November 1, 2019 01:09

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 5001+ and removed 10.rebuild-darwin: 11-100 10.rebuild-linux: 2501-5000 labels Nov 1, 2019

jtojnar approved these changes Nov 1, 2019

View reviewed changes

jtojnar merged commit ad1f06f into NixOS:staging Nov 1, 2019

jtojnar added a commit that referenced this pull request Nov 1, 2019

gnome-doc-utils: do not propagate Python

dafa543

That breaks packages that rely on Python like itstool does. #72342 (comment)

This was referenced Nov 1, 2019

nixos/qemu-vm: Option to use squashfs Nix store closure instead of virtfs access to host's store #72354

Closed

itstool 2.0.4 crashes when freeing xmlDocs itstool/itstool#17

Open

chkno deleted the itstool-2.0.6 branch November 4, 2019 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

itstool: 2.0.2 -> 2.0.6 #72342

itstool: 2.0.2 -> 2.0.6 #72342

chkno commented Oct 31, 2019

chkno commented Oct 31, 2019

jtojnar commented Oct 31, 2019

chkno commented Oct 31, 2019

chkno commented Oct 31, 2019

FRidh commented Oct 31, 2019

chkno commented Oct 31, 2019 •

edited

chkno commented Oct 31, 2019

jtojnar commented Oct 31, 2019

FRidh commented Oct 31, 2019 •

edited

jtojnar commented Oct 31, 2019 •

edited

jtojnar commented Oct 31, 2019

chkno commented Nov 1, 2019

jtojnar commented Nov 1, 2019

jtojnar commented Nov 1, 2019

jtojnar left a comment

jtojnar commented Nov 1, 2019

itstool: 2.0.2 -> 2.0.6 #72342

itstool: 2.0.2 -> 2.0.6 #72342

Conversation

chkno commented Oct 31, 2019

Motivation for this change

Things done

Notify maintainers

chkno commented Oct 31, 2019

jtojnar commented Oct 31, 2019

chkno commented Oct 31, 2019

chkno commented Oct 31, 2019

FRidh commented Oct 31, 2019

chkno commented Oct 31, 2019 • edited

chkno commented Oct 31, 2019

jtojnar commented Oct 31, 2019

FRidh commented Oct 31, 2019 • edited

jtojnar commented Oct 31, 2019 • edited

jtojnar commented Oct 31, 2019

chkno commented Nov 1, 2019

jtojnar commented Nov 1, 2019

jtojnar commented Nov 1, 2019

jtojnar left a comment

Choose a reason for hiding this comment

jtojnar commented Nov 1, 2019

chkno commented Oct 31, 2019 •

edited

FRidh commented Oct 31, 2019 •

edited

jtojnar commented Oct 31, 2019 •

edited