Update Package request for libservicelog

Bug #1705538 reported by bugproxy on 2017-07-20
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
The Ubuntu-power-systems project
High
Canonical Foundations Team
libservicelog (Ubuntu)
High
Steve Langasek
Xenial
High
Łukasz Zemczak

Bug Description

[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>

bugproxy (bugproxy) on 2017-07-20
tags: added: architecture-ppc64 bugnameltc-156869 severity-high targetmilestone-inin16044
Changed in ubuntu:
assignee: nobody → Ubuntu on IBM Power Systems Bug Triage (ubuntu-power-triage)
affects: ubuntu → libservicelog (Ubuntu)
Changed in ubuntu-power-systems:
importance: Undecided → High
tags: added: upgrade-software-version
Changed in ubuntu-power-systems:
assignee: nobody → Canonical Foundations Team (canonical-foundations)
tags: added: triage-g
Manoj Iyer (manjo) on 2017-08-28
Changed in libservicelog (Ubuntu):
assignee: Ubuntu on IBM Power Systems Bug Triage (ubuntu-power-triage) → Canonical Foundations Team (canonical-foundations)
importance: Undecided → High
tags: added: triage-r
removed: triage-g
Steve Langasek (vorlon) on 2017-08-29
Changed in libservicelog (Ubuntu):
assignee: Canonical Foundations Team (canonical-foundations) → Steve Langasek (vorlon)
status: New → Fix Committed
Changed in libservicelog (Ubuntu Xenial):
assignee: nobody → Canonical Foundations Team (canonical-foundations)
Steve Langasek (vorlon) wrote :

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) wrote :

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

Changed in libservicelog (Ubuntu):
status: Fix Committed → Fix Released
Manoj Iyer (manjo) wrote :

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?

tags: added: triage-a
removed: triage-r
Steve Langasek (vorlon) wrote :

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.

------- 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) wrote :

------- 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) wrote :

------- 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

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.

------- 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

tags: added: id-59a4c1e54b49b47985c45376
Manoj Iyer (manjo) on 2017-10-16
tags: added: triage-g
removed: triage-a
Dimitri John Ledkov (xnox) wrote :

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?

Changed in libservicelog (Ubuntu Xenial):
status: New → Incomplete
bugproxy (bugproxy) wrote :

------- 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) wrote :

------- 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) wrote :

------- 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.

tags: added: triage-a
removed: triage-g
Changed in ubuntu-power-systems:
status: New → Incomplete
status: Incomplete → Triaged
Manoj Iyer (manjo) on 2017-11-28
Changed in libservicelog (Ubuntu Xenial):
importance: Undecided → High
tags: added: triage-r
removed: triage-a
Steve Langasek (vorlon) on 2017-12-18
Changed in libservicelog (Ubuntu Xenial):
status: Incomplete → Triaged
Manoj Iyer (manjo) on 2018-01-16
tags: added: triage-g
removed: triage-r
Manoj Iyer (manjo) on 2018-02-05
tags: added: triage-a
removed: triage-g
Changed in libservicelog (Ubuntu Xenial):
assignee: Canonical Foundations Team (canonical-foundations) → Łukasz Zemczak (sil2100)
status: Triaged → In Progress
Łukasz Zemczak (sil2100) wrote :

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

description: updated
Steve Langasek (vorlon) wrote :

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.

Changed in libservicelog (Ubuntu Xenial):
status: In Progress → Incomplete
Łukasz Zemczak (sil2100) wrote :

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!

Manoj Iyer (manjo) on 2018-02-26
Changed in ubuntu-power-systems:
status: Triaged → Incomplete
Patricia Gaughen (gaughen) wrote :

This work is currently blocked waiting on a test case.

bugproxy (bugproxy) wrote :

------- 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) wrote :

> 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?

Manoj Iyer (manjo) on 2018-03-19
tags: added: triage-g
removed: triage-a
Robie Basak (racb) wrote :

> [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) wrote :

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

bugproxy (bugproxy) wrote :

------- 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

Steve Langasek (vorlon) on 2018-04-14
Changed in libservicelog (Ubuntu Xenial):
status: Incomplete → Triaged
Changed in ubuntu-power-systems:
status: Incomplete → Triaged

Default Comment by Bridge

Default Comment by Bridge

Robie Basak (racb) wrote :

> 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.

------- 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

description: updated
Łukasz Zemczak (sil2100) wrote :

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) wrote :

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

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!

Changed in libservicelog (Ubuntu Xenial):
status: Triaged → Fix Committed
tags: added: verification-needed verification-needed-xenial
Changed in ubuntu-power-systems:
status: Triaged → Fix Committed
Łukasz Zemczak (sil2100) wrote :

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.

------- 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) wrote :

------- 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

tags: added: verification-done verification-done-xenial
removed: verification-needed verification-needed-xenial
Launchpad Janitor (janitor) wrote :

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

Changed in libservicelog (Ubuntu Xenial):
status: Fix Committed → Fix Released

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.

Changed in ubuntu-power-systems:
status: Fix Committed → Fix Released

------- 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

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers