Merge ~bryce/ubuntu/+source/bash:ubuntu/cosmic-devel into ubuntu/+source/bash:ubuntu/cosmic-devel

Proposed by Bryce Harrington on 2019-06-07
Status: Merged
Approved by: Andreas Hasenack on 2019-06-11
Approved revision: a02cca6ea0ef358cb75b35721ad83040eaba28b6
Merge reported by: Bryce Harrington
Merged at revision: a02cca6ea0ef358cb75b35721ad83040eaba28b6
Proposed branch: ~bryce/ubuntu/+source/bash:ubuntu/cosmic-devel
Merge into: ubuntu/+source/bash:ubuntu/cosmic-devel
Diff against target: 174 lines (+152/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/bash44-020.diff (+144/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack 2019-06-07 Approve on 2019-06-11
Canonical Server Team 2019-06-07 Pending
Ubuntu Server Dev import team 2019-06-07 Pending
Review via email: mp+368573@code.launchpad.net

Description of the change

Backport of an upstream fix to a cpu hang when waiting on background tasks.

PPA available for testing is at
https://launchpad.net/~bryce/+archive/ubuntu/bash-sru-19-010-1

I was not able to reproduce the hang myself, but apparently it can take days or weeks to reproduce. However, the fix was taken upstream and looks clearly correct to me. I'm confident given enough run time it would reproduce. It may be difficult to positively test for the bug's absence however.

I'm assuming that the target branch for this should be ubuntu/cosmic-devel; I couldn't find documentation to this effect however it seems to be what others are targeting to. Let me know if it's wrong.

I'm unclear if any tags are needed for SRU bugs but am gathering that none are. Again, let me know if otherwise.

To post a comment you must log in.
Andreas Hasenack (ahasenack) wrote :

Just the upload tag is needed, and that will be handled at that time. Regarding the target branch, for SRUs ubuntu/<release>-devel is correct :)

Reviewing now.

Andreas Hasenack (ahasenack) wrote :

I'd just suggest a different branch name next time, as there is a cosmic-devel already, and it's too generic :) But it works for the purposes of the fix, of course.

Andreas Hasenack (ahasenack) wrote :

I see you changed the upstream patch to the unified format, just like others have done in the past (bash44-019 for example).

I compared the context diff (upstream) and unified diff (as being added here) and they are equal.

Usually when we change the upstream patch, we also take the opportunity to remove "noise", like the git index markers ("index fc96603..2684632 100644") and the --show-c-func bit ("@@ -812,8 +812,22 @@ bgp_add (pid, status)") <-- "bgp_add (pid, status)". This is inline with the recommendation at https://wiki.debian.org/UsingQuilt#Using_.quiltrc_configuration_file. But in this case, you didn't really change the patch in terms of code, just in terms of format. So our rule has been that it can stay as it, but it's the first time we came across a format change :)

If you want to change it, since the patch applied cleanly, I think just a sequence of "quilt push -a; quilt refresh bash44-020.diff" would do it, if you have that quiltrc config file.

I would just like to ask you to add a few DEP3 headers, that can be done below the description. Something like this:

--- a/debian/patches/bash44-020.diff
+++ b/debian/patches/bash44-020.diff
@@ -15,6 +15,10 @@ processes, it is possible for the hash table bash uses to store exit
 statuses from asynchronous processes to develop loops. This patch fixes
 the loop causes and adds code to detect any future loops.

+Origin: upstream, https://ftp.gnu.org/gnu/bash/bash-4.4-patches/bash44-020
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/bash/+bug/1822776
+Last-Update: 2019-06-10
+
 diff --git a/jobs.c b/jobs.c
 index fc96603..2684632 100644
 --- a/jobs.c

Andreas Hasenack (ahasenack) wrote :

Ops, missed one bit in d/changelog, no need to use -proposed. See comment inline.

Bryce Harrington (bryce) wrote :

Thanks for the review Andreas, I've updated the branch with the suggested changes.

I'll use the "sru.1822776-bionic" format for branch names going forward, I'd intended to use that here but got my parameters confused I guess.

Andreas Hasenack (ahasenack) wrote :

+1

review: Approve
Andreas Hasenack (ahasenack) wrote :

I'm gonna push the upload tag for a02cca6ea0ef358cb75b35721ad83040eaba28b6, and you dput:

$ git push pkg upload/4.4.18-2ubuntu3.1
Enumerating objects: 16, done.
Counting objects: 100% (16/16), done.
Delta compression using up to 2 threads
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 2.73 KiB | 558.00 KiB/s, done.
Total 11 (delta 7), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/bash
 * [new tag] upload/4.4.18-2ubuntu3.1 -> upload/4.4.18-2ubuntu3.1

Bryce Harrington (bryce) wrote :

Thanks, pushed via dput.

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 48c78c4..fbd8fae 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+bash (4.4.18-2ubuntu3.1) cosmic; urgency=medium
7+
8+ * d/p/bash44-020.diff: Add fix for hang on 'wait' statement
9+ (LP: #1822776)
10+
11+ -- Bryce Harrington <bryce@canonical.com> Thu, 06 Jun 2019 17:54:43 -0700
12+
13 bash (4.4.18-2ubuntu3) cosmic; urgency=medium
14
15 * Resurrect "Set the default path to comply with Debian policy" in
16diff --git a/debian/patches/bash44-020.diff b/debian/patches/bash44-020.diff
17new file mode 100644
18index 0000000..340d085
19--- /dev/null
20+++ b/debian/patches/bash44-020.diff
21@@ -0,0 +1,144 @@
22+ BASH PATCH REPORT
23+ =================
24+
25+Bash-Release: 4.4
26+Patch-ID: bash44-020
27+
28+Bug-Reported-by: Graham Northup <northug@clarkson.edu>
29+Bug-Reference-ID: <537530c3-61f0-349b-9de6-fa4e2487f428@clarkson.edu>
30+Bug-Reference-URL: http://lists.gnu.org/archive/html/bug-bash/2017-02/msg00025.html
31+
32+Bug-Description:
33+
34+In circumstances involving long-running scripts that create and reap many
35+processes, it is possible for the hash table bash uses to store exit
36+statuses from asynchronous processes to develop loops. This patch fixes
37+the loop causes and adds code to detect any future loops.
38+
39+Origin: upstream, https://ftp.gnu.org/gnu/bash/bash-4.4-patches/bash44-020
40+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/bash/+bug/1822776
41+Last-Update: 2019-06-10
42+
43+diff --git a/jobs.c b/jobs.c
44+index fc96603..2684632 100644
45+--- a/jobs.c
46++++ b/jobs.c
47+@@ -812,8 +812,22 @@ bgp_add (pid, status)
48+ ps_index_t *bucket, psi;
49+ struct pidstat *ps;
50+
51+- bucket = pshash_getbucket (pid);
52+- psi = bgp_getindex ();
53++ /* bucket == existing chain of pids hashing to same value
54++ psi = where were going to put this pid/status */
55++
56++ bucket = pshash_getbucket (pid); /* index into pidstat_table */
57++ psi = bgp_getindex (); /* bgpids.head, index into storage */
58++
59++ /* XXX - what if psi == *bucket? */
60++ if (psi == *bucket)
61++ {
62++#ifdef DEBUG
63++ internal_warning ("hashed pid %d (pid %d) collides with bgpids.head, skipping", psi, pid);
64++#endif
65++ bgpids.storage[psi].pid = NO_PID; /* make sure */
66++ psi = bgp_getindex (); /* skip to next one */
67++ }
68++
69+ ps = &bgpids.storage[psi];
70+
71+ ps->pid = pid;
72+@@ -841,32 +855,47 @@ pshash_delindex (psi)
73+ ps_index_t psi;
74+ {
75+ struct pidstat *ps;
76++ ps_index_t *bucket;
77+
78+ ps = &bgpids.storage[psi];
79+ if (ps->pid == NO_PID)
80+ return;
81+
82+- if (ps->bucket_next != NO_PID)
83++ if (ps->bucket_next != NO_PIDSTAT)
84+ bgpids.storage[ps->bucket_next].bucket_prev = ps->bucket_prev;
85+- if (ps->bucket_prev != NO_PID)
86++ if (ps->bucket_prev != NO_PIDSTAT)
87+ bgpids.storage[ps->bucket_prev].bucket_next = ps->bucket_next;
88+ else
89+- *(pshash_getbucket (ps->pid)) = ps->bucket_next;
90++ {
91++ bucket = pshash_getbucket (ps->pid);
92++ *bucket = ps->bucket_next; /* deleting chain head in hash table */
93++ }
94++
95++ /* clear out this cell, just in case */
96++ ps->pid = NO_PID;
97++ ps->bucket_next = ps->bucket_prev = NO_PIDSTAT;
98+ }
99+
100+ static int
101+ bgp_delete (pid)
102+ pid_t pid;
103+ {
104+- ps_index_t psi;
105++ ps_index_t psi, orig_psi;
106+
107+ if (bgpids.storage == 0 || bgpids.nalloc == 0 || bgpids.npid == 0)
108+ return 0;
109+
110+ /* Search chain using hash to find bucket in pidstat_table */
111+- for (psi = *(pshash_getbucket (pid)); psi != NO_PIDSTAT; psi = bgpids.storage[psi].bucket_next)
112+- if (bgpids.storage[psi].pid == pid)
113+- break;
114++ for (orig_psi = psi = *(pshash_getbucket (pid)); psi != NO_PIDSTAT; psi = bgpids.storage[psi].bucket_next)
115++ {
116++ if (bgpids.storage[psi].pid == pid)
117++ break;
118++ if (orig_psi == bgpids.storage[psi].bucket_next) /* catch reported bug */
119++ {
120++ internal_warning ("bgp_delete: LOOP: psi (%d) == storage[psi].bucket_next", psi);
121++ return 0;
122++ }
123++ }
124+
125+ if (psi == NO_PIDSTAT)
126+ return 0; /* not found */
127+@@ -904,15 +933,22 @@ static int
128+ bgp_search (pid)
129+ pid_t pid;
130+ {
131+- ps_index_t psi;
132++ ps_index_t psi, orig_psi;
133+
134+ if (bgpids.storage == 0 || bgpids.nalloc == 0 || bgpids.npid == 0)
135+ return -1;
136+
137+ /* Search chain using hash to find bucket in pidstat_table */
138+- for (psi = *(pshash_getbucket (pid)); psi != NO_PIDSTAT; psi = bgpids.storage[psi].bucket_next)
139+- if (bgpids.storage[psi].pid == pid)
140+- return (bgpids.storage[psi].status);
141++ for (orig_psi = psi = *(pshash_getbucket (pid)); psi != NO_PIDSTAT; psi = bgpids.storage[psi].bucket_next)
142++ {
143++ if (bgpids.storage[psi].pid == pid)
144++ return (bgpids.storage[psi].status);
145++ if (orig_psi == bgpids.storage[psi].bucket_next) /* catch reported bug */
146++ {
147++ internal_warning ("bgp_search: LOOP: psi (%d) == storage[psi].bucket_next", psi);
148++ return -1;
149++ }
150++ }
151+
152+ return -1;
153+ }
154+diff --git a/patchlevel.h b/patchlevel.h
155+index a711c49..4a65dc0 100644
156+--- a/patchlevel.h
157++++ b/patchlevel.h
158+@@ -25,6 +25,6 @@
159+ regexp `^#define[ ]*PATCHLEVEL', since that's what support/mkversion.sh
160+ looks for to find the patch level (for the sccs version string). */
161+
162+-#define PATCHLEVEL 19
163++#define PATCHLEVEL 20
164+
165+ #endif /* _PATCHLEVEL_H_ */
166diff --git a/debian/patches/series b/debian/patches/series
167index ee570e7..dab8a2a 100644
168--- a/debian/patches/series
169+++ b/debian/patches/series
170@@ -22,3 +22,4 @@ bzero.diff
171 man-macro-warnings.diff
172 po-de-fix.diff
173 man-vx-opts.diff
174+bash44-020.diff

Subscribers

People subscribed via source and target branches