Update Package request for libservicelog

[bugproxy]

[Impact]

Update Package request for libservicelog. Some upstream patches have been requested to be included in both bionic and xenial. The addressed issues can potentially affect customer environments, causing possible segmentation faults.

[Test Case]

The packages will be validated by the requesting party (see original description).

Besides making sure servicelog does not segfault anymore and performing general dogfooding, the upstream test-suite will need to be run and made sure to pass on the affected systems.

[Regression Potential]

Hard to assess but the requested patches have been in upstream trunk since at least a year and no issues have been reported - changes also present in Debian and Ubuntu bionic since last year.

[Original Description]

---Problem Description---
Update Package request for libservicelog

Machine Type = lpar

---Steps to Reproduce---
 servicelog --dump

---uname output---
Linux tuleta4u-lp9 4.10.0-27-generic #30~16.04.2-Ubuntu SMP Thu Jun 29 16:06:52 UTC 2017 ppc64le ppc64le ppc64le GNU/Linux

Please pull below mentioned patches for libservicelog package

commit 4fe9d9239f172604e16268ca1fb6fff1c06632b2
Author: Ankit Kumar <email address hidden>
Date: Sat May 20 01:38:41 2017 +0530

    Validate text string before and after bind call

    While binding string to query, if string is NULL then bind call gets ignored.
    While fetching data from string, if data is NULL then current code makes query
    as NULL and returns.

    This patch validates text string before and after bind call. It assigns string
    to "" incase data is NULL points to NULL pointer. After this we will be able
    to display information even if some of string data is NULL.

    It also does NULL checks for compulsory string.

    Signed-off-by: Ankit Kumar <email address hidden>
    [Killed redundant goto statements - Vasant]
    Signed-off-by: Vasant Hegde <email address hidden>

commit 787594814eb88e3149cd660bcb6aaa6d3dd1347c
Author: Ankit Kumar <email address hidden>
Date: Sat May 20 01:10:49 2017 +0530

    Correct string length calculation and validates destination buffer size before strncpy

    This patch corrects string length calculation logic and validates destination
    buffer size before calling strncpy to avoid memory corruption.

    Signed-off-by: Ankit Kumar <email address hidden>
    [Moved memset to right place and removed redundant condition check -
     Vasant]
    Signed-off-by: Vasant Hegde <email address hidden>

commit 48875ee8614eeefaa3d5d8ff92fb424915738169
Author: Ankit Kumar <email address hidden>
Date: Thu Sep 15 16:16:49 2016 +0530

    NULL check before strdup call

    This patch does "NULL checks" before passing argument to strdup call.

    Signed-off-by: Ankit Kumar <email address hidden>
    Signed-off-by: Vasant Hegde <email address hidden>

commit 40b4f7a52e61fb9da30b4cb9b5de9a85673da262
Author: Ankit Kumar <email address hidden>
Date: Thu Sep 15 16:16:48 2016 +0530

    NULL check before strlen call

    This patch checks NULL pointer before strlen call.

    Signed-off-by: Ankit Kumar <email address hidden>
    [Fixed build warning - Vasant]
    Signed-off-by: Vasant Hegde <email address hidden>

[Steve Langasek (vorlon)]

For 17.10, I've attempted to upload the 1.1.18 upstream version rather than cherry-picking individual fixes. However, the package fails to build on the Ubuntu builders due to a series of bugs.

 - when the test binary is built, it is linked against libservicelog.so, not libservicelog.la. This means that there is no libtool wrapper script to set the path; and the test binary only ever works if there is an already-installed version of libservicelog on the system path which is being tested instead of the just-built version.
 - if servicelog_open() returns a failure, test_libservicelog segfaults because it tries to dereference a null pointer.
 - if /var/lib/libservicelog/ does not already exist on the system, servicelog_open() will *always* fail.

I've written patches for the first two issues; but the hard-coding of a system path in the library makes it difficult to fix the third issue in a way that is compatible with an unprivileged build environment.

I'm attaching the two patches, and will disable the test suite in the Ubuntu package pending resolution of the third issue. This is not a regression since the test suite is new since 1.1.16.

[Launchpad Janitor (janitor)]

This bug was fixed in the package libservicelog - 1.1.18-0ubuntu2

---------------
libservicelog (1.1.18-0ubuntu2) artful; urgency=medium

  * Disable the test suite, which doesn't actually test the built library
    but a previously-installed library (which we don't have on the buildds).

 -- Steve Langasek <email address hidden> Mon, 28 Aug 2017 21:40:49 -0700

[Manoj Iyer (manjo)]

Steve, Since this package is in universe for Xenial, would it be possible to sync this packages with debian for 16.04.4 once the patches have been picked up in Debian?

[Steve Langasek (vorlon)]

No. The SRU guidelines are not different for universe packages vs. main. For xenial, the plan here would still be to cherry-pick the patches.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-09-12 10:39 EDT-------
Ankit,

did you have a chance to look at Steve's comment#5 where he found some issues with libservice log? We need to have they fixed for package upgrade.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-09-13 00:16 EDT-------
(In reply to comment #11)
> Ankit,
>
> did you have a chance to look at Steve's comment#5 where he found some
> issues with libservice log? We need to have they fixed for package upgrade.

Hi Breno,

libservicelog test cases are for development purpose, not for validating installed libservicelog package on distro.

Hence I don't think , we need to push this changes to distros.

But will fix this in upstream libservicelog.

@Vasant: Do you agree on this ?

~Ankit

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-09-20 07:02 EDT-------
Steve,

(In reply to comment #5)
> For 17.10, I've attempted to upload the 1.1.18 upstream version rather than
> cherry-picking individual fixes. However, the package fails to build on the
> Ubuntu builders due to a series of bugs.
>
> - when the test binary is built, it is linked against libservicelog.so, not
> libservicelog.la. This means that there is no libtool wrapper script to set
> the path; and the test binary only ever works if there is an
> already-installed version of libservicelog on the system path which is being
> tested instead of the just-built version.
> - if servicelog_open() returns a failure, test_libservicelog segfaults
> because it tries to dereference a null pointer.
> - if /var/lib/libservicelog/ does not already exist on the system,
> servicelog_open() will *always* fail.
>
> I've written patches for the first two issues;

Thanks for the patch. I didn't see your s-o-b in patch. Do you have any objection in adding your s-o-b and merging these patches in upstream?

> system path in the library makes it difficult to fix the third issue in a
> way that is compatible with an unprivileged build environment.

yes. That's an issue. I need to see how we can fix this issue.

-Vasant

[Steve Langasek (vorlon)]

On Wed, Sep 20, 2017 at 11:09:56AM -0000, bugproxy wrote:
> (In reply to comment #5)
> > For 17.10, I've attempted to upload the 1.1.18 upstream version rather than
> > cherry-picking individual fixes. However, the package fails to build on the
> > Ubuntu builders due to a series of bugs.

> > - when the test binary is built, it is linked against libservicelog.so, not
> > libservicelog.la. This means that there is no libtool wrapper script to set
> > the path; and the test binary only ever works if there is an
> > already-installed version of libservicelog on the system path which is being
> > tested instead of the just-built version.
> > - if servicelog_open() returns a failure, test_libservicelog segfaults
> > because it tries to dereference a null pointer.
> > - if /var/lib/libservicelog/ does not already exist on the system,
> > servicelog_open() will *always* fail.

> > I've written patches for the first two issues;

> Thanks for the patch. I didn't see your s-o-b in patch. Do you have any
> objection in adding your s-o-b and merging these patches in upstream?

The two patches are attached to the bug log in Launchpad:

https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1705538/+attachment/4940508/+files/0001-Use-libtool-dependencies-for-linking-tests.patch
https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1705538/+attachment/4940509/+files/0002-Fix-segfault-on-servicelog_open-failure.patch

No objections to these being merged upstream. If you need something formal
from Canonical regarding copyright / license, please let me know.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-09-21 02:39 EDT-------
Steve,

> The two patches are attached to the bug log in Launchpad:
>
> https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1705538/
> +attachment/4940508/+files/0001-Use-libtool-dependencies-for-linking-tests.
> patch
> https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1705538/
> +attachment/4940509/+files/0002-Fix-segfault-on-servicelog_open-failure.patch

Ok

>
> No objections to these being merged upstream. If you need something formal
> from Canonical regarding copyright / license, please let me know.

Its Open Source project with GPL/LGPL license. No formal process required from anyone. I just need s-o-b from author. Since you are agreeing, I will add your s-o-b and merge this code.

Thanks!
-Vasant

[Dimitri John Ledkov (xnox)]

This package is out of date in debian w.r.t. upstream. Is Frederic Bonnard going to upgrade the package to 1.1.18?

Do you still require NULL check / NULL dereference patches SRUed into xenial?

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-10-18 09:25 EDT-------
Adding Fred to this bug, so he can see the request to upgrade the package in Debian.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-10-24 10:07 EDT-------
(In reply to comment #17)
> This package is out of date in debian w.r.t. upstream. Is Frederic Bonnard
> going to upgrade the package to 1.1.18?
>
> Do you still require NULL check / NULL dereference patches SRUed into xenial?

Yes please .

-Vasant

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-10-24 11:15 EDT-------
Hi,
I pushed the last 1.1.18 package to Debian.
Few differences with ubuntu current 1.1.18-0ubuntu2.

F.

[Łukasz Zemczak (sil2100)]

Uploaded to the xenial UNAPPROVED queue, waiting for SRU team review.

[Steve Langasek (vorlon)]

This SRU says that the updated package will be "validated by the requesting party". How will IBM validate it? If IBM have a test plan for how we will make sure this update is correct, we should be able to run that test plan ourselves. If IBM do not have such a test plan, I am concerned that this SRU will stall in the process, with no clear ownership of approving the package update as tested.

[Łukasz Zemczak (sil2100)]

As per Steve's comment - could someone provide a more detailed test case for the uploaded changes? e.g. how to reproduce the original issues in steps that could be executed by a potential tester.

Thank you!

[Patricia Gaughen (gaughen)]

This work is currently blocked waiting on a test case.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-03-01 09:53 EDT-------
(In reply to comment #27)
> This work is currently blocked waiting on a test case.

Sorry for delay in response..

We use servcelog command and rtas_errd daemon (ppc64-diag) package to verify libservicelog.

Also we have unit test cases in libservicelog package .. which is enough to cover basic validation.

(Of course Steve had reported/fixed few issues in running test cases. that's all available in upstream now).

Let us know if you need more detailed plan here.

-Vasant

[Steve Langasek (vorlon)]

> Also we have unit test cases in libservicelog package .. which is enough to cover basic
> validation.

If I'm not mistaken, we still have this testsuite disabled in the package because while I sent some fixes, servicelog_open() still fails if /var/lib/servicelog does not exist. Does this mean you will plan to run this testsuite manually against the packages in the SRU?

[Robie Basak (racb)]

> [Impact]

> Update Package request for libservicelog. Some upstream patches have been requested to be included in both bionic and xenial.

This is not a user impact explanation as described at https://wiki.ubuntu.com/StableReleaseUpdates#Procedure and I can't find any other justification or need for this to go to Xenial.

Please could someone update the reason this need to go into Xenial from a user perspective and update the bug description accordingly?

[Patricia Gaughen (gaughen)]

We're waiting on a response to Steve's question in comment #20.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-04-12 09:24 EDT-------
Steve,

(In reply to comment #29)
> > Also we have unit test cases in libservicelog package .. which is enough to cover basic
> > validation.
>
> If I'm not mistaken, we still have this testsuite disabled in the package
> because while I sent some fixes, servicelog_open() still fails if
> /var/lib/servicelog does not exist. Does this mean you will plan to run
> this testsuite manually against the packages in the SRU?

Yes. Unfortunately we still need to disable test suites.

Its in my TODO list .. but never got their due to various reasons. So we have 2 issues that blocks us enabling test suite by default.

1 - if servicelog_open() returns a failure, test_libservicelog segfaults because it tries to dereference a null pointer.

I've applied your patch. I think we still left with one more place. I have patch locally to fix this issue.

2 - if /var/lib/libservicelog/ does not already exist on the system, servicelog_open() will *always* fail.

I'm thinking of creating database under current dir itself and make it work. I will get this done in a day or two.

-Vasant

------- Comment From <email address hidden> 2018-04-12 09:28 EDT-------
> > Update Package request for libservicelog. Some upstream patches have been requested to be included in both bionic and xenial.
>
> This is not a user impact explanation as described at
> https://wiki.ubuntu.com/StableReleaseUpdates#Procedure and I can't find any
> other justification or need for this to go to Xenial.
>
> Please could someone update the reason this need to go into Xenial from a
> user perspective and update the bug description accordingly?

Patches mentioned in this bugzilla can potentially impact customer environment. Hence we wanted those fixes in 16.04 LTS release.

But when we added test cases we did broke it in build environment. Anyway this was new stuff added later.

Let me know whether you can do some manually test and push changes?
-OR- give test build so that IBM can test it before you merge? and I promise to test it ASAP .

-Vasant

[bugproxy (bugproxy)]

Default Comment by Bridge

[bugproxy (bugproxy)]

Default Comment by Bridge

[Robie Basak (racb)]

> Patches mentioned in this bugzilla can potentially impact customer environment.

How? Ubuntu's SRU procedure requires this to be documented to make sure that the SRU is justified.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-05-17 07:10 EDT-------
>
> > Patches mentioned in this bugzilla can potentially impact customer environment.
>
> How? Ubuntu's SRU procedure requires this to be documented to make sure that
> the SRU is justified.

In some case we may hit segfaults (these are regression issues).

-Vasant

[Łukasz Zemczak (sil2100)]

I have updated the description to include some of the information that has been shared through the comments. One thing I wanted to know: is running the test-suite manually work correctly right now? Is it possible to do that and get valid test results?

Also Vasant, you mentioned that you have a local patch that fixes running the test-suite on build time? Is that correct? Do we want to include those as part of this SRU? (would have to be also released for cosmic though).

[Steve Langasek (vorlon)]

Running the test suite manually seems to me that it should be sufficient here.

[Steve Langasek (vorlon)]

Hello bugproxy, or anyone else affected,

Accepted libservicelog into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libservicelog/1.1.16-0ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Łukasz Zemczak (sil2100)]

Thank you!

Vasant, new version of libservicelog is now available in xenial-proposed - could you proceed with testing and forward us the results? Testing + running the test-suite manually.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-06-15 09:37 EDT-------
(In reply to comment #41)
> Thank you!
>
> Vasant, new version of libservicelog is now available in xenial-proposed -
> could you proceed with testing and forward us the results? Testing + running
> the test-suite manually.

Sure.. Will get back to you with the result early next week.

Thanks!

-Vasant

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-06-15 10:50 EDT-------
(In reply to comment #42)
> (In reply to comment #41)
> > Thank you!
> >
> > Vasant, new version of libservicelog is now available in xenial-proposed -
> > could you proceed with testing and forward us the results? Testing + running
> > the test-suite manually.
>
> Sure.. Will get back to you with the result early next week.

We have verified -proposed package and looks good.

-Vasant

[Launchpad Janitor (janitor)]

This bug was fixed in the package libservicelog - 1.1.16-0ubuntu1.1

---------------
libservicelog (1.1.16-0ubuntu1.1) xenial; urgency=medium

  * debian/patches: Cherry-pick and rebase upstream fixes for additional NULL
    checking, string length calculation and validation. (LP: #1705538)
    - 0001-Correct-string-length-calculation-and-validates-dest.patch
    - 0002-NULL-check-before-strdup-call.patch
    - 0003-NULL-check-before-strlen-call.patch
    - 0004-Validate-text-string-before-and-after-bind-call.patch

 -- Łukasz 'sil2100' Zemczak <email address hidden> Wed, 21 Feb 2018 17:44:12 +0100

[Chris Halse Rogers (raof)]

The verification of the Stable Release Update for libservicelog has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2018-06-22 11:15 EDT-------
(In reply to comment #44)
> This bug was fixed in the package libservicelog - 1.1.16-0ubuntu1.1

Thanks!

-Vasant