[MIR] rygel

[Sebastien Bacher]

* Availability

Builds on all supported architectures in Ubuntu and on sync from Debian

* Rationale

We would like to enable dlna sharing of media files, which is a GNOME upstream feature and relying on rygel.

The binaries to promoted would be
librygel-core-2.6-2
librygel-db-2.6-2
librygel-renderer-2.6-2
librygel-server-2.6-2
rygel

* Security

No CVE/known security issue

* Quality assurance

- the desktop-packages team is subscribed to the package
- upstream has a testsuit which is not being used during build, we are going to look at changing that

* Dependendies

The package requires gupnp-dlna (MIR bug #1785649) and gupnp-av (MIR bug #1785629). gupnp was in main but was demoted since (bug #1799974 in case a new review is needed), same for gssdp (bug #1799977)

* Standards compliance

the package is using standard packaging (dh11), the standards-version is 4.1.1, the package is in sync from Debian

* Maintainance

Upstream is active and the desktop team is going to look after the package in ubuntu

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Didier Roche-Tolomelli (didrocks)]

* There are a lot of crash reports in launchpad. Those are several years old and shouldn't apply anymore. Maybe a little bit of cleanup of really old one, and looking at more recent ones will help once supported to review real new crashes (between launchpad and errors.ubuntu.com)?
* What is going to pull rygel on the iso: directly seeded or a recommends/deps somewhere? Note that there is as well rygel-tracker which shouldn't be promoted until tracker situation is deciphered.
* Some lintian warnings would be great to looked at, namely:
- W: rygel-2.6-dev: pkg-config-unavailable-for-cross-compilation (multiple of thm)
- W: rygel: uses-implicit-await-trigger interest /usr/lib/rygel-2.6 (line 1)
- W: rygel: uses-implicit-await-trigger interest rygel-restart (line 2)
* I guess rygel-preferences isn't going to be promoted, correct? (maybe we should list exactly the packages that are going to be promoted). It has a dep as linking on libgssdp-1.0-3, which is in universe.
* However rygel binary package has some dep issues as well:
 - dep on libgssdp-1.0-3 (gssdp source in universe) and libgupnp-1.0-4 (gupnp source in universe), which are not listed in the current MIR for the stack.
 - 2 recommends with its source in universe and not listed to be MIRed. They should be downgraded to Suggests, if possible: gstreamer1.0-libav and gstreamer1.0-plugins-ugly
* If we want to promote rygel-2.6-dev to main, there is again the libgupnp-1.0-dev dep as well which needs to be sorted out.
* librygel-server-2.6-2 is a dep of rygel, and depends as well on libgssdp-1.0-3 and libgupnp-1.0-4
* librygel-renderer-2.6-2 is a dep of rygel, and depends as well on libgupnp-1.0-4
* librygel-core-2.6-2 is a dep of rygel, and depends as well on libgssdp-1.0-3 and libgupnp-1.0-4
* some copyright are missing:
  - 2008-2009 Florian Brosch
  - 2012 Choe Hwanjin <email address hidden>
  - 2013 Cable Television Laboratories, Inc.
  - 2014 Jens Georg <email address hidden>
  - 2014 Atlantic PuffinPack AB.
  - 2017 Samuel CUELLA]
  - Intel Corporation copyright should be extended to 2013
  - Jens Georg copyright should be extended to 2016
I may have missed others but that should be about it.

Opened question:
- tests are ran during package build, do we want autopkgtests (unsure if those are just unit tests or not, but it would maybe prevent at least vala regression)?
- the doc is in the -dev package. That's ok with me, weird to have different standards considering debian maintainer is the same than the other packages though.

I think there is a bunch of work to be done before going to another review or give a conditional +1 on that one. I'll defer also to the security team once the package is a little bit more ready from the pure MIR perspective.

[Sebastien Bacher (seb128)]

> * There are a lot of crash reports in launchpad. Those are several years old and shouldn't apply anymore.
> Maybe a little bit of cleanup of really old one, and looking at more recent ones will help once supported
> to review real new crashes (between launchpad and errors.ubuntu.com)?

Done, the launchpad bugs are down to 7 & e.u.c is looks without real issues.

> * What is going to pull rygel on the iso: directly seeded or a recommends/deps somewhere?
> Note that there is as well rygel-tracker which shouldn't be promoted until tracker situation is deciphered.

gnome-control-center has the sharing UI, we plan to add the Recommends to the service there

> * Some lintian warnings would be great to looked at, namely:
> - W: rygel-2.6-dev: pkg-config-unavailable-for-cross-compilation (multiple of thm)

That's because the package is not using a multiarch location, do you consider that a pre-requirement to get the MIR approved? It's probably not too difficult to change the build but it requires some testing, etc.

> - W: rygel: uses-implicit-await-trigger interest /usr/lib/rygel-2.6 (line 1)
> - W: rygel: uses-implicit-await-trigger interest rygel-restart (line 2)

Fixed in 0.36.2-2 just uploaded to Debian (autosync to come)

> * I guess rygel-preferences isn't going to be promoted, correct?
> (maybe we should list exactly the packages that are going to be promoted). It has a dep as linking on libgssdp-1.0-3, which is in universe.

Correct, gnome-control-center is the UI to control the service in our usecase

> * However rygel binary package has some dep issues as well:
> - dep on libgssdp-1.0-3 (gssdp source in universe) and libgupnp-1.0-4 (gupnp source in universe), which are not listed in the current MIR for the stack.

Right, those used to be in main but new MIRs filed now to have a new look before re-promoting
https://bugs.launchpad.net/ubuntu/+source/gupnp/+bug/1799974
https://bugs.launchpad.net/ubuntu/+source/gssdp/+bug/1799977

> - 2 recommends with its source in universe and not listed to be MIRed.
> They should be downgraded to Suggests, if possible: gstreamer1.0-libav and gstreamer1.0-plugins-ugly

Right, those needs to be demoted but let's keep the package in sync for now it's going to lower the work until the other MIRs and this one are approved.

> * If we want to promote rygel-2.6-dev to main, there is again the libgupnp-1.0-dev dep as well which needs to be sorted out.

I don't think we need the -dev in main since we can build on universe. Also gupnp has a MIR in review.

> * librygel-server-2.6-2 is a dep of rygel, and depends as well on libgssdp-1.0-3 and libgupnp-1.0-4
> * librygel-renderer-2.6-2 is a dep of rygel, and depends as well on libgupnp-1.0-4
> * librygel-core-2.6-2 is a dep of rygel, and depends as well on libgssdp-1.0-3 and libgupnp-1.0-4

Right, those 2 MIRs are pre-requirements now

> * some copyright are missing:
> - 2008-2009 Florian Brosch
> - 2012 Choe Hwanjin <email address hidden>
> - 2013 Cable Television Laboratories, Inc.
> - 2014 Jens Georg <email address hidden>
> - 2014 Atlantic PuffinPack AB.
> - 2017 Samuel CUELLA]
> - Intel Corporation copyright should be extended to 2013
> - Jen...

[Launchpad Janitor (janitor)]

This bug was fixed in the package rygel - 0.36.2-2

---------------
rygel (0.36.2-2) unstable; urgency=medium

  * Some tweaks following the Ubuntu MIR reviews (lp: #1786489)
  * debian/copyright: updated the list of copyright owners
  * debian/rygel.triggers: use noawait triggers

 -- Sebastien Bacher <email address hidden> Mon, 03 Dec 2018 16:36:28 +0100

[Didier Roche-Tolomelli (didrocks)]

Ok, so to sum up, remaining work to do:
- Add multi-arch to the libs
- (optional) Run as autopkgtests to ensure vala doesn't break this build

Once the above is addressed (multi-arch), +1 from the MIR team.
I think the security team can start right now reviewing this package and the rdepends.

[Sebastien Bacher (seb128)]

The multi-arching is done now (and synced to disco)

We discussed the autopkgtest rebuild thing on IRC and decided that it was probably best serve to have vala itself including the rebuild of some packages in its tests, and then use a ppa to test rebuild things with new versions before upload

[Chris Coulson (chrisccoulson)]

I reviewed rygel 0.36.2-5ubuntu1 as checked in to eoan. This isn't a full security audit, but rather a quick gauge of maintainability.

- rygel is a UPnP AV media server, allowing audio and video to be shared with other devices. It can also operate as a media renderer which can be controlled by UPnP controllers.
- rygel is part of the GNOME project.
- rygel is written in vala.
- The website claims it us under active development, but activity seems to have slowed somewhat this year - since February, the only git commits are mostly translation updates and what look like minor build / bug fixes.
- Of the 118 issues currently open in gitlab, many of them appear to have been originally imported from Bugzilla and haven't had much activity since the migration.
- No CVEs in our database.
- Build dependencies in main, except for:
  - libgssdp-1.2-dev (bug 1799977)
  - libgupnp-1.2-dev, libgupnp-av-1.0-dev, libgupnp-dlna-2.0-dev (bug 1799974)
  - valac (this doesn't create any binary package dependencies)
- The rygel binary package recommends gstreamer1.0-plugins-ugly, which is in universe.

- I ran coverity, although this mostly highlights problems in C code that is auto-generated by valac.
- Coverity shows up quite a few unreachable code issues, like this one in rygel_base_configuration_real_get_interface:
...
    g_propagate_error (error, _inner_error0_);
    return NULL;
    return result;
}

And also rygel_basic_management_test_real_run_co:
...
    }
    g_object_unref (_data_->_async_result);
    return FALSE;
    g_task_return_pointer (_data_->_async_result, _data_, NULL);
    if (_data_->_state_ != 0) {
        while (!_data_->_task_complete_) {
...

These look like vala bugs.

- There's a fd leak in rygel_energy_management_get_mac_and_network_type if fd == 0. I'm not sure if this is a real issue - it's only possible to hit this condition if stdin is closed.
- Coverity catches what looks like an obvious null deref here in rygel_ruih_service_manager_real_constructed in the second call to block1_data_unref:
...
        if (G_UNLIKELY (_inner_error0_ != NULL)) {
            _g_object_unref0 (config_dir_file);
            block1_data_unref (_data1_);
            _data1_ = NULL;
            if (_inner_error0_->domain == RYGEL_RUIH_SERVICE_ERROR) {
                goto __catch2_rygel_ruih_service_error;
            }
            if (_inner_error0_->domain == G_IO_ERROR) {
                goto __catch2_g_io_error;
            }
            _g_object_unref0 (config_dir_file);
            block1_data_unref (_data1_);
            _data1_ = NULL;
            _g_free0 (ui_listing_directory);
            g_critical ("file %s: line %d: unexpected error: %s (%s, %d)", __FILE__, __LINE__, _inner_error0_->message, g_quark_to_string (_inner_error0_->domain), _inner_error0_->code);
            g_clear_error (&_inner_error0_);
            return;
        }
...

I'm not sure if this is actually a vala bug though.

- Rygel.SimpleDataSource.run uses lseek without checking the return value.
- The uint.clamp() call in Rygel.External.Container constuctor generates C code that does a negative unsigned compare. Would "uint.min(child_count, int.MAX)" be more appropriate th...

[Jens Georg (yg-jensge)]

Are you going to file upstream tickets for the relevant findings?

[Sebastien Bacher (seb128)]

@Jens, yes I plan to do that (thanks for keeping an eye on launchpad btw ;-)

[Mathieu Trudel-Lapierre (cyphermox)]

Current state appears to be that Desktop Team should look into the Recommends (gstreamer plugins-ugly), and someone have a review of the XML parser options (as pointed out in the security review, specifically for NOENT and RECOVER... maybe NONET should be added?).

[Jens Georg (yg-jensge)]

Actually I completely mis-understood NOENT - it does the exact opposite of what I thought it does ...

[Sebastien Bacher (seb128)]

The Rygel recommends on gst-plugins-ugly has been lowered to a suggest, the review problems have been upstreamed and upstream acked the NOENT issue here. Sending back to the MIR team, we will backport the NOENT fix but otherwise I think it should be good to be acked on the MIR side now

[Didier Roche-Tolomelli (didrocks)]

Now that all remarks have been addressed, I'm happy to give an official +1 from the MIR team side.

[Matthias Klose (doko)]

see https://people.canonical.com/~ubuntu-archive/component-mismatches-proposed.svg
rygel tries to pull in 30+ other packages into main.

[Sebastien Bacher (seb128)]

Sorry, it was a leftover recommends on gstreamer libav, downgraded to a suggests now

[Didier Roche-Tolomelli (didrocks)]

+1 on the MIR side