Ticket #134 (closed task: fixed)

Opened 5 years ago

Last modified 4 years ago

Approval needed - zsync needs to ship internal zlib for rsync compatibility

Reported by: robert Owned by:
Priority: major Keywords: meeting
Cc: cassmodiah@…, toshio, robatino, kparal Blocked By:
Blocking:

Description

Shipping a system library internally of another package is usually a no-go for Fedora. Unluckily zsync is shipping a modified version of zlib internally. In order to get zsync into Fedora, I'm hereby requesting a FESCO approval for this to make an official exception and to know the situation for further similar situations.

The modified internal zlib inside of zsync is required for on-the-wire binary compatibility to rsync and other zsync versions.

I'm not a C developer, but I asked Thorsten Glaser (from MirBSD and mksh) about externalizing the difference between the modified zlib in order to be able to link against the system zlib rather the modified internal one. He told me, that he had the same issue at MirBSD, but that it isn't possible to solve this so far. If we would change things, we would make the results of zsync binary incompatible to other rsync and zsync versions. According to him, we've here the same situation as at rsync itself which also seems to ship an internal zlib. If I understood him correct, the modification of zlib makes zlib just a lossy compression, means, that the next N bytes won't sent.

https://bugzilla.redhat.com/show_bug.cgi?id=490140 is the zsync Fedora packaging review request.

Change History

comment:1 Changed 5 years ago by toshio

  • Cc toshio added

FPC, not FESCo comment:

This is a bad idea. We're creating a longer chain of dependencies and rebuilds here without any tracking of the dependencies. For instance, a security hole is discovered in zlib-1.2.3. They fix. We zero day update. No dependency exists between rsync and zlib, therefore it's likely that we won't know that rsync is affected until the rsync upstream realizes they are affected and issues their own security fix. Once again, no dependency exists linking rsync and zsync so we could end up waiting until the zsync author notices they're affected and fixes their code.

This change would also make zsync depend on rsync for other things. If the zlib in rsync is updated (say they find another way to optimize their version of the library), we won't be informed of the change until the zsync author notices it.

Also note that librsync has this note:

abo says

It [carrying a modified zlib] does get better compression, but at a price. I actually think that getting the code to a point where a feature like this can be easily added or removed is more important than the feature itself. Having generic pre and post processing layers for hit/miss data would be useful. I would not like to see it added at all if it tangled and complicated the code.

It also doesn't require a modified zlib... pysync uses the standard zlib to do it by compressing the data, then throwing it away. I don't know how much benefit the rsync modifications to zlib actually are, but if I was implementing it I would stick to a stock zlib until it proved significantly better to go with the fork.

So this seems to be less about interoperability and more about optimizing. If zsync and rsync need this optimization, the zlib from rsync should be rebuilt as a dynamic library with a new name. Then both programs can link against it (as well as librsync and pysync if they choose). The best thing for us in that scenario is for the rsync upstream to agree and want to manage the work of forking this new library. Please ask them if they're willing to do that.

comment:2 Changed 5 years ago by robert

So you are hereby willing to do the necessary work and provide the needed C knowledge to solve this issue sane and you will fix the upcoming bugs caused by such an action?

comment:3 Changed 5 years ago by toshio

Are you saying that as maintainer of a package, you are not? Then what makes you qualified to maintain the package? Put another way, if you are not willing to do this separation, then how can you claim to be able to support the library-is-embedded-in-source-fork which is more work?

And finally, have you asked the rsync upstream if they're willing to maintain the fork? That was the best case scenario I asked you to work towards first.

comment:4 follow-ups: ↓ 5 ↓ 7 Changed 5 years ago by robert

I think, I speak for many Fedora contributors when saying, that many of them are just packaging the software they actually use. (and just use and not more). Thus many of the Fedora packagers surely don't have a heavily extended C programming or libtool or autotools knowledge. Your words would mean, that they shouldn't maintain their packages then because they're not qualified enough.

On the other hand, the Fedora Project is always measuring in two different ways: Before and after the review. Nobody IMHO really has a deep look what after a review happens to a package and whether such no-gos are silently introduced - just from by personal point of view.

And even in case of zero day security issues, the updates are not always happening immediately at Fedora when e.g. looking to the current clamav CVEs where neither the package maintainer nor the Fedora Security team have pushed fixed packages for F-9 and F-10 until now. So IMHO it doesn't matter whether we ship libraries internal in rare cases or not - the important thing is, whether the security updates are driven at all or whether they simply don't happen!

If you are speaking from the security aspect, I have to remember you to Lubomir Rintel's words at the fedora-packaging-list: "Well, not much of an issue if it has a caring and responsive maintainer. The Security Response team tracks also embedded and old copies and notifies their maintainers" - you remember his recent e-mail?

comment:5 in reply to: ↑ 4 Changed 5 years ago by toshio

Replying to robert:

I think, I speak for many Fedora contributors when saying, that many of them are just packaging the software they actually use. (and just use and not more). Thus many of the Fedora packagers surely don't have a heavily extended C programming or libtool or autotools knowledge. Your words would mean, that they shouldn't maintain their packages then because they're not qualified enough.

Uhm... All packagers who are working on packages that use autotools need to be willing to dive into programming at the libtool/autotools level. They don't have to be good at it, but they have to be willing to learn more about it. Building packages is just too intertwined with that to turn a blind eye anytime an issue occurs there.

On the other hand, the Fedora Project is always measuring in two different ways: Before and after the review. Nobody IMHO really has a deep look what after a review happens to a package and whether such no-gos are silently introduced - just from by personal point of view.

This is a flaw in our system that needs to be addressed by more work, not less. We don't have enough manpower interested in reviewing to handle all the new packages and checking up on what's happening with the old packages. Propose a solution.

And even in case of zero day security issues, the updates are not always happening immediately at Fedora when e.g. looking to the current clamav CVEs where neither the package maintainer nor the Fedora Security team have pushed fixed packages for F-9 and F-10 until now. So IMHO it doesn't matter whether we ship libraries internal in rare cases or not - the important thing is, whether the security updates are driven at all or whether they simply don't happen!

We'll disagree here. Striving for excellence doesn't mean perfection. But it does mean not introducing more problems than already exist.

If you are speaking from the security aspect, I have to remember you to Lubomir Rintel's words at the fedora-packaging-list: "Well, not much of an issue if it has a caring and responsive maintainer. The Security Response team tracks also embedded and old copies and notifies their maintainers" - you remember his recent e-mail?

If you're going to make this argument then you're in the wrong forum. This is a generic argument rather than one specific to zsync. So instead of asking for a single exception to the Guidelines for a single package, propose that the packaging committee drops the requirement for libraries to be shipped separately. Get information from other distributions that allows bundling and static linking. Find people who agree with you so that you can discuss this on list and then show up at the FPC meeting where it's decided to express your views.

comment:6 Changed 5 years ago by robert

I won't request your last suggestion regarding dropping the requirement. Because I just think, there are cases like here at zsync, where an exception would make sense. This is what I initially requested and where I would like to see an official FESCO reply - I thought, my initial request was not that misleading that much...

comment:7 in reply to: ↑ 4 Changed 5 years ago by toshio

Replying to robert:

If you are speaking from the security aspect, I have to remember you to Lubomir Rintel's words at the fedora-packaging-list: "Well, not much of an issue if it has a caring and responsive maintainer. The Security Response team tracks also embedded and old copies and notifies their maintainers" - you remember his recent e-mail?

Also, it seems as if I haven't been following my email as well as I should since this:

https://www.redhat.com/archives/fedora-packaging/2009-April/msg00021.html

Pretty much negates the earlier email.

comment:8 Changed 5 years ago by toshio

As to having a specific exception for zsync, you stated that the reason it needs one is to have on-the-wire compatibility with rsync. I've given information that says on-the-wire compatibility can be achieved using the system zlib in comment 2:

It also doesn't require a modified zlib... pysync uses the standard zlib to do it by compressing the data, then throwing it away.

If zsync wants to use a non-system zlib for optimization or even because that comment is not correct, I've also told you how to go about achieving that by forking the library. If you lack the skill to do that, then you can find someone who can (for instance, the rsync upstream that I said were the ideal case, or the zsync upstream). This is the same as any other problem that comes up in a Fedora review where the packager relies on a third party (hopefully upstream) to resolve the issue. For instance, we can't include Testopia in Fedora because of licensing conflicts. Someone, whether a packager or upstream, has to write code to resolve the issue before it can be included.

comment:9 Changed 5 years ago by jstanley

If I'm not mistaken, we disallowed this, right?

comment:10 Changed 5 years ago by cassmodiah

  • Keywords meeting added

I really want a decision, this issue is outstanding for 2 months.

comment:11 Changed 5 years ago by toshio

Note also:

https://bugzilla.redhat.com/show_bug.cgi?id=495310

Robert: what happened when you asked the rsync upstream about officially forking zlib instead of carrying a version locally inside of rsync?

comment:12 Changed 5 years ago by jstanley

  • Resolution set to fixed
  • Status changed from new to closed

Sorry for the delay in closing this ticket out - I thought it was pending something, that's why it never made it's way back to the agenda. Anyhow, it was there last week, and we decided two things:

  • zsync may not ship an embedded zlib
  • rsync may likewise not ship an embedded zlib

This obviously means that rsync must be repaired - this was missed in the merge review, that's the only reason it made it this far. Now that it's been brought to our attention..... :) Simo said that getting rsync/zlib into shape to use the system zlib and not break the on-wire format should not be hard.

comment:13 follow-up: ↓ 14 Changed 5 years ago by petersen

For what it's worth a simple patch to the make files commenting out the internal zlib deps seems to work ok.

https://bugzilla.redhat.com/show_bug.cgi?id=490140#c4

comment:14 in reply to: ↑ 13 Changed 5 years ago by petersen

Replying to petersen:

For what it's worth a simple patch to the make files commenting out the internal zlib deps seems to work ok.

https://bugzilla.redhat.com/show_bug.cgi?id=490140#c4

Unfortunately it is not that simple and I was not testing correctly: I think naive patch is a no-go.

comment:15 Changed 5 years ago by petersen

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening since cassmodiah says he has some new ideas.

comment:16 Changed 5 years ago by jstanley

What are these new ideas? I don't see any in either the review or here.

comment:17 Changed 5 years ago by sharkcz

The rsync bug (https://bugzilla.redhat.com/show_bug.cgi?id=495310) is updated with 2 patches to build and distribute the internal forked copy of zlib as a shared library.

comment:18 Changed 5 years ago by sharkcz

I have talked to Ivana (she is the zlib maintainer), sent her all relevant information and she has promised to review the differences in both zlib forks and give us an opinion what's doable and what's not.

comment:19 Changed 5 years ago by jstanley

  • Resolution set to fixed
  • Status changed from reopened to closed

FESCo reaffirmed the decision that neither rsync nor zlib may ship an internal copy of zlib. To this end, sharkcz is working on splitting the internal zlib provided by rsync into a shared library which can be used by both rsync and zsync (and any other application requiring compatibility with rsync).

comment:20 Changed 4 years ago by varekova

The upstream maintainer of zlib wrote he is willing to discuss about adding of rsync functionality of zlib to zlib package, if there will be some changes (now the discussion starts - email forward to zlib fedora maintainer). In case of zsync: zlib upstream maintainers suggest to substitute the zsync hack of zlib and use different functions which already are in zlib and should do the same work (just resend the e-mail to the person who started to add zsync to fedora).

comment:21 Changed 4 years ago by robatino

  • Cc robatino added

comment:22 Changed 4 years ago by kparal

  • Cc kparal added
Note: See TracTickets for help on using tickets.