xpra icon
Bug tracker and wiki

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#960 closed defect (fixed)

0.15.4: don't detect LZ4 any more

Reported by: onlyjob Owned by: onlyjob
Priority: major Milestone: 0.16
Component: core Version: 0.15.x
Keywords: Cc:

Description

It seems that "safer lz4 version checking code" in 0.15.4 broke detection of LZ4 on Debian. 0.15.3 show "lz4" in "Packet Compressors" but not 0.15.4. Please advise.

Change History (19)

comment:1 Changed 5 years ago by onlyjob

Reverting r10042 fixes the problem. Version of "python-lz4" on Debian is "0.7.0+dfsg-2".

comment:2 Changed 5 years ago by Antoine Martin

Owner: changed from Antoine Martin to onlyjob

Should be fixed in r10440.

FYI: the version stuff was merged into python-lz4 over a year ago: https://github.com/steeve/python-lz4/issues/19.
In trunk we have completely removed the ugly hack that tries to figure out the missing version number.
We will require the version patch, or preferably this version: https://github.com/steeve/python-lz4/pull/41, and if upstream does not merge it within a reasonable time, we will switch to this improved "fork" instead.

You can quickly test using this test script:

$ python xpra/net/compression.py
lz4                 : True
lz4.version         : 0.7.0
lzo                 : True
lzo.version         : 2.08
zlib                : True
zlib.version        : 1.0

@onlyjob: if this works for you, please close.

comment:3 Changed 5 years ago by onlyjob

Thanks, I test it later.
FYI "python-lz4" on Debian is using system "liblz4" as static linking with bundled libs is against our policy primarily due to security concerns ("+dfsg" in version indicates modifications to original source tarball).

Debian's "python-lz4" is based upon https://pypi.python.org/pypi/lz4/0.7.0
I'm not sure if we need to switch it to fork just yet... After all it is not vulnerable, right?

comment:4 Changed 5 years ago by Antoine Martin

The github "fork" / "pull-request" avoids static linking amongst other things. Those are already pull-requests for upstream as https://github.com/steeve/python-lz4 is upstream, same as the pypi link.

You can assume that it is not vulnerable since it is not statically linked (rely on liblz4 system updates to take care of that), but "vulnerable" is not the only concern here: we want a version that is actively maintained.

comment:5 Changed 5 years ago by onlyjob

I've just tried it but r10440 did not fix this regression unfortunately...

comment:6 Changed 5 years ago by onlyjob

It is quite frustrating that ​https://github.com/steeve/python-lz4 do not tag releases ("latest" release is "0.3.2" from 2012)... I reckon I could crerry-pick a version patch for Debian package...

comment:7 Changed 5 years ago by Antoine Martin

Something in the debian packaging strips the version from the egg path:

  • on a plain install from source of 0.7.0 (here on an up-to-date jessie test system):
    $ python -c "import lz4;print(lz4.__file__)"
    /root/.python-eggs/lz4-0.7.0-py2.7-linux-i686.egg-tmp/lz4.so
    
  • with the packaged version from the repos:
    $ python -c "import lz4;print(lz4.__file__)"
    /usr/lib/python2.7/dist-packages/lz4.so
    

So we have no way of knowing what version is installed at all, or if it is safe to use or not.

r10443 "deals" with this by printing a warning and hoping for the best.

comment:8 Changed 5 years ago by onlyjob

On Debian, because of linkage with system library, version-based assumptions are meaningless so it seems right to not assume anything if version can't be extracted. However I'm confused why r10443 factor this change as standalone patch? Are we expected to carry this patch downstream? Why make this behaviour OS-dependent? Wouldn't it be reasonable to always avoid making assumptions about unknown version of python-lz4?

comment:9 Changed 5 years ago by Antoine Martin

Resolution: fixed
Status: newclosed

version-based assumptions are meaningless


The new code in trunk checks the version of liblz4 when it is exposed (which is the right one to check), but there is no version information at all in the current Debian python-lz4 package.


it seems right to not assume anything


We try our best to figure out if things are safe giving the restrictions imposed by the versions shipped elsewhere.


Why make this behaviour OS-dependent?


Because to the best of my knowledge, other distros don't have this problem.


Wouldn't it be reasonable to always avoid making assumptions about unknown version of python-lz4?


We don't make assumptions, we try to figure out if things are safe.

If you have better code for doing so, please submit it.

comment:10 Changed 5 years ago by onlyjob

Because to the best of my knowledge, other distros don't have this problem.


It is not a distro problem when unreliable method of detecting version of a version-less module does not work. Clearly there are two problems -- one in python-lz4 and another in the attempted way to detect version of python-lz4.

FYI just uploaded "python-lz4_0.7.0+dfsg-3" exposes version information.

Fair enough, I do not have a better code but I'd like to note that this regression was introduced in the attempt to fix what was not broken on Debian in first place. It is not Xpra's job to advise user about vulnerabilities in python-lz4 or any other module. IMHO if it were only a warning like it should have been, there would be no regression at all. I understand intention to select best compressor but it is wrong to assume the worst when version detection fails.

comment:11 Changed 5 years ago by Antoine Martin

It is not a distro problem when unreliable method of detecting version of a version-less module does not work.


Like I said: other distros do not have this problem, they do not need these ugly workarounds.


that this regression was introduced in the attempt to fix what was not broken on Debian in first place
It is not Xpra's job to advise user about vulnerabilities in python-lz4 or any other module.


Not your decision I am afraid.
And since ditros ship outdated and buggy versions of various libraries, we will continue to point this out. Especially when those libraries are network facing and can lead to a full system compromise as easily as the lz4 one.

Now can you please stop beating this dead horse?
I don't have time to argue the merits of packaging it the Debian way every time something breaks because of it.

If you don't like the patch, don't use it.

comment:12 Changed 5 years ago by Jiang

Per ticket #936, I installed lz4 in python via pip, which install it at
/usr/local/lib/python2.7/dist-packages/lz4.so
because in ubuntu trusty, there is apparently no python-lz4 package. Before 0.15.4 (e.g. in 0.15.3), when I tell it --compressor=lz4, xpra find lz4 and use it just fine. Somehow it cannot do so anymore in 0.15.4.

Perhaps xpra can add /usr/local/lib/python2.7/dist-packages/ to one of the place it looks for lz4 packages?

comment:13 Changed 5 years ago by Jiang

Per ticket #936, this problem persist in trunk (0.16) beta.

comment:14 Changed 5 years ago by Antoine Martin

this problem persist in trunk (0.16) beta.


That's right: trunk dropped compatibility with non versioned packages of lz4, the ugly hack has been removed.

comment:15 Changed 5 years ago by Jiang

In the latest version (0.15.5) lz4 works, albeit with a persistent warning unknown version of "python-lz4". So everything is fine, except it'll be great if there's a way to silence the warning... Even when I'm just toggling sound on and off it spits out such a warning! But this does not affect function, it merely annoys...

comment:16 Changed 5 years ago by Antoine Martin

To silence the warning, use a more recent version which includes the version information. Like this one: http://xpra.org/src/python-lz4-0.8.0-rc1.tar.xz.

When moving to 0.16, those older versions will no longer be supported.

comment:17 Changed 5 years ago by Jiang

Yes, after upgrading lz4 to this version, the warning disappear. I do not see any performance improvement. Is this upgrade simply for the sake of code maintenance or is there some new features introduced?

comment:18 Changed 5 years ago by Antoine Martin

I do not see any performance improvement.
Is this upgrade simply for the sake of code maintenance or is there some new features introduced?


Both.

We got rid of the ugly version guessing code: that's the code maintenance part.
New features: there will be some changes in 0.16, taking advantage of the newer lz4 features (if present) so that we can lower CPU consumption and use more bandwidth.
See Sampling, or a faster LZ4

comment:19 Changed 5 years ago by Antoine Martin

As per this lz4 detection mailing list post:
The installed version of lz4 can be obtained without the "ugly hack" in
xpra/net/compression.py by:

import pkg_resources
pkg_resources.get_distribution("lz4").version

which returns "0.7.0" even when lz4 is (correctly) installed to /usr/lib/python2.7/site-packages/lz4.so.

Applied to all supported branches (inc trunk) in r11010.

Note: See TracTickets for help on using tickets.