Merge ~ahasenack/ubuntu/+source/ipmitool:disco-segfault-1817307 into ubuntu/+source/ipmitool:ubuntu/devel

Proposed by Andreas Hasenack
Status: Merged
Approved by: Andreas Hasenack
Approved revision: 5ba32b6586a15ceec83696dc55327476408b573c
Merged at revision: 5ba32b6586a15ceec83696dc55327476408b573c
Proposed branch: ~ahasenack/ubuntu/+source/ipmitool:disco-segfault-1817307
Merge into: ubuntu/+source/ipmitool:ubuntu/devel
Diff against target: 91 lines (+57/-1)
4 files modified
debian/changelog (+7/-0)
debian/control (+2/-1)
debian/patches/0130-fix-segfault.patch (+47/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack Approve
Christian Ehrhardt  (community) Needs Fixing
Review via email: mp+363810@code.launchpad.net

Description of the change

Patch found in upstream's git repo, and confirmed by the reporter to fix the segfault. I don't have a way to reproduce it atm, though.

Reporter will try other releases. Based on the version, it should at least happen in bionic as well. I will prepare branches for those once this lands, and if we get confirmation of the problem in the other ubuntu releases.

The upstream patch applies with offset, but I checked and the change is inserted in the correct places. Because of that I chose to not refresh the patch and keep it like it was committed upstream. If you prefer, however, I can refresh it and use our normal full DEP3 header instead of the github "add .patch to the url" trick.

To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Generally LGTM, but one thing - the patch has no dep-3 headers at all.
It is just the upstream patch as is.

Once that is fixed feel free to self-approve and upload as the rest really is ok.

Furthermore since they seem to release [1] rareley or lets call it sporadic let us bother Manoj to also report that to Debian.

[1]: https://github.com/pjdhpe/ipmitool/releases

review: Needs Fixing
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I did the "bothering" :-)

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Hm, I forgot to push, uups

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

There

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

self +1

review: Approve
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Tagged and uploaded:
$ git push pkg upload/1.8.18-6ubuntu1
Enumerating objects: 21, done.
Counting objects: 100% (21/21), done.
Delta compression using up to 4 threads
Compressing objects: 100% (15/15), done.
Writing objects: 100% (15/15), 2.61 KiB | 667.00 KiB/s, done.
Total 15 (delta 10), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/ipmitool
 * [new tag] upload/1.8.18-6ubuntu1 -> upload/1.8.18-6ubuntu1

$ dput ubuntu ../ipmitool_1.8.18-6ubuntu1_source.changes
Checking signature on .changes
gpg: ../ipmitool_1.8.18-6ubuntu1_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../ipmitool_1.8.18-6ubuntu1.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading ipmitool_1.8.18-6ubuntu1.dsc: done.
  Uploading ipmitool_1.8.18-6ubuntu1.debian.tar.xz: done.
  Uploading ipmitool_1.8.18-6ubuntu1_source.buildinfo: done.
  Uploading ipmitool_1.8.18-6ubuntu1_source.changes: done.
Successfully uploaded packages.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

LGTM now, thanks!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 5c14961..be80ee8 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+ipmitool (1.8.18-6ubuntu1) disco; urgency=medium
7+
8+ * d/p/0130-fix-segfault.patch: Correct lanplus segment violation for
9+ truncated response. (LP: #1817307)
10+
11+ -- Andreas Hasenack <andreas@canonical.com> Thu, 28 Feb 2019 11:21:41 -0300
12+
13 ipmitool (1.8.18-6build1) disco; urgency=medium
14
15 * No-change rebuild for readline soname change.
16diff --git a/debian/control b/debian/control
17index 2b6d4c3..eb59734 100644
18--- a/debian/control
19+++ b/debian/control
20@@ -1,7 +1,8 @@
21 Source: ipmitool
22 Section: utils
23 Priority: optional
24-Maintainer: Jörg Frings-Fürst <debian@jff.email>
25+Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
26+XSBC-Original-Maintainer: Jörg Frings-Fürst <debian@jff.email>
27 Build-Depends:
28 debhelper (>> 11),
29 libncurses-dev,
30diff --git a/debian/patches/0130-fix-segfault.patch b/debian/patches/0130-fix-segfault.patch
31new file mode 100644
32index 0000000..c8aaa06
33--- /dev/null
34+++ b/debian/patches/0130-fix-segfault.patch
35@@ -0,0 +1,47 @@
36+From 815aae70cf8dc9f0e1ba1923fc4ec3cc16d0d2f1 Mon Sep 17 00:00:00 2001
37+From: pjdhpe <44778156+pjdhpe@users.noreply.github.com>
38+Date: Wed, 28 Nov 2018 07:27:29 -0600
39+Subject: [PATCH] Correct lanplus segment violation for truncated response.
40+
41+Correct segment violation in lanplus from dcmi truncated response.
42+
43+ This corrects Issue #72.
44+ On occasion a dcmi power reading will return error C6, and a
45+ truncated response payload. As the decrypted payload is shorter
46+ than the expected length, lanplus_decrypt_aes_cbc_128() adjusts
47+ the payload_size downward by one byte. In ipmi_lan_poll_single()
48+ the calculation to determine if the payload size has increased
49+ erroniously sets extra_data_length to -1, with a subsequent
50+ segv when calling a memmove to shift response data.
51+ The fix is to check for a positive value in the extra_data_length.
52+
53+Origin: upstream, https://github.com/pjdhpe/ipmitool/commit/815aae70cf8dc9f0e1ba1923fc4ec3cc16d0d2f1
54+Bug: https://github.com/ipmitool/ipmitool/issues/72
55+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/ipmitool/+bug/1817307
56+Last-Update: 2019-02-28
57+---
58+ src/plugins/lanplus/lanplus.c | 4 ++--
59+ 1 file changed, 2 insertions(+), 2 deletions(-)
60+
61+diff --git a/src/plugins/lanplus/lanplus.c b/src/plugins/lanplus/lanplus.c
62+index b7dfc3d9..d3e4e91b 100644
63+--- a/src/plugins/lanplus/lanplus.c
64++++ b/src/plugins/lanplus/lanplus.c
65+@@ -792,7 +792,7 @@ ipmi_lan_poll_single(struct ipmi_intf * intf)
66+ * rsp->data_len becomes the length of that data
67+ */
68+ extra_data_length = payload_size - (offset - payload_start) - 1;
69+- if (extra_data_length) {
70++ if (extra_data_length > 0) {
71+ rsp->data_len = extra_data_length;
72+ memmove(rsp->data, rsp->data + offset, extra_data_length);
73+ } else {
74+@@ -846,7 +846,7 @@ ipmi_lan_poll_single(struct ipmi_intf * intf)
75+ }
76+ read_sol_packet(rsp, &offset);
77+ extra_data_length = payload_size - (offset - payload_start);
78+- if (rsp && extra_data_length) {
79++ if (rsp && extra_data_length > 0) {
80+ rsp->data_len = extra_data_length;
81+ memmove(rsp->data, rsp->data + offset, extra_data_length);
82+ } else {
83diff --git a/debian/patches/series b/debian/patches/series
84index 80f7a3c..c4a885b 100644
85--- a/debian/patches/series
86+++ b/debian/patches/series
87@@ -9,3 +9,4 @@
88 0110-getpass-prototype.patch
89 0115-typo.patch
90 0125-nvidia-iana.patch
91+0130-fix-segfault.patch

Subscribers

People subscribed via source and target branches