Merge ~lucaskanashiro/ubuntu/+source/keepalived:sru-lp1960036 into ubuntu/+source/keepalived:ubuntu/focal-devel

[Lucas Kanashiro]

Fix LP #1960036, the SRU template is already in place and I confirmed the new version works as expected. I uploaded the package to the PPA below:

https://launchpad.net/~lucaskanashiro/+archive/ubuntu/testing/+packages

This package contains no DEP-8 test.

[Sergio Durigan Junior (sergiodj)]

I'm taking a look at this one.

[Sergio Durigan Junior (sergiodj)]

Thanks for the MP, Lucas.

The SRU template looks good; thanks for including detailed info on how to reproduce the bug.

I think something is missing here. I was able to reproduce the problem locally, but the package you provided on your PPA doesn't seem to fix it. After installing it, I still don't see the quorum being gained again:

# add-apt-repository ppa:lucaskanashiro/testing

 More info: https://launchpad.net/~lucaskanashiro/+archive/ubuntu/testing
Press [ENTER] to continue or Ctrl-c to cancel adding it.

Hit:1 http://security.ubuntu.com/ubuntu focal-security InRelease
Hit:2 http://archive.ubuntu.com/ubuntu focal InRelease
Get:3 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal InRelease [18.1 kB]
Hit:4 http://archive.ubuntu.com/ubuntu focal-updates InRelease
Hit:5 http://archive.ubuntu.com/ubuntu focal-backports InRelease
Get:6 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal/main amd64 Packages [656 B]
Get:7 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal/main Translation-en [512 B]
Fetched 19.2 kB in 1s (21.9 kB/s)
Reading package lists... Done
# apt update
Hit:1 http://archive.ubuntu.com/ubuntu focal InRelease ...

review: Needs Fixing

[Lucas Kanashiro (lucaskanashiro)]

That's weird. I tested it locally and after installing the new package and restarting (stop and then start) the nginx service it regained quorum. I'll be setting up a new test environment to see what's happening.

[Lucas Kanashiro (lucaskanashiro)]

Sergio, I re-tested it and it got fixed for me. I updated the Test Plan section in the bug description to try to make more clear what I've done.

Let me summary here. Below are the steps I performed after I reproduced the bug:

root@keepalived-debug2:~# add-apt-repository ppa:lucaskanashiro/testing

 More info: https://launchpad.net/~lucaskanashiro/+archive/ubuntu/testing
Press [ENTER] to continue or Ctrl-c to cancel adding it.

Hit:1 http://archive.ubuntu.com/ubuntu focal InRelease
Get:2 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal InRelease [18.1 kB]
Hit:3 http://security.ubuntu.com/ubuntu focal-security InRelease
Hit:4 http://archive.ubuntu.com/ubuntu focal-updates InRelease
Hit:5 http://archive.ubuntu.com/ubuntu focal-backports InRelease
Get:6 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal/main amd64 Packages [656 B]
Get:7 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal/main Translation-en [512 B]
Fetched 19.2 kB in 2s (11.2 kB/s)
Reading package lists... Done
root@keepalived-debug2:~# dpkg -l keepalived
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name Version Architecture Description
+++-==============-===================-============-===============================================
ii keepalived 1:2.0.19-2ubuntu0.1 amd64 Failover and monitoring daemon for LVS clusters
root@keepalived-debug2:~# apt install keepalived
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following packages will be upgraded:
  keepalived
1 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
Need to get 364 kB of archives.
After this operation, 0 B of additional disk space will be used.
Get:1 http://ppa.launchpad.net/lucaskanashiro/testing/ubuntu focal/main amd64 keepalived amd64 1:2.0.19-2ubuntu0.2~ppa1 [364 kB]
Fetched 364 kB in 2s (228 kB/s)
(Reading database ... 76070 files and directories currently installed.)
Preparing to unpack .../keepalived_1%3a2.0.19-2ubuntu0.2~ppa1_amd64.deb ...
Unpacking keepalived (1:2.0.19-2ubuntu0.2~ppa1) over (1:2.0.19-2ubuntu0.1) ...
Setting up keepalived (1:2.0.19-2ubuntu0.2~ppa1) ...
Processing triggers for man-db (2.9.1-1) ...
Processing triggers for dbus (1.12.16-2ubuntu2.1) ...
Processing triggers for systemd (245.4-4ubuntu3.15) ...
root@keepalived-debug2:~# dpkg -l keepalived
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name Version Architecture Description
+++-==============-========================-============-===============================================
ii keepalived 1:2.0.19-2ubuntu0.2~ppa1 amd64 Failover and monitoring daemon for LVS clusters
root@keepalived-debug2:~# journalctl -u keepalived | grep Quorum
Feb 08 14:46:47 keepalived-debug2 Keepalived_vrrp[8079]: Quorum lost for tracked process track_nginx
Feb 08...

[Sergio Durigan Junior (sergiodj)]

Thanks, Lucas.

I know what happened now. I was testing with "systemctl restart nginx", but that doesn't work because the restart process happens too fast and keepalived isn't able to catch the change of state. When I used stopped and then started the process using separate commands, I could finally verify that keepalived was properly detecting the lost and regaining of quorum. Sorry about the noise.

With that out of the way, I'm happy with the MP now. I'm leaving a minor cosmetic comment on the changelog, but other than that I'm +1. Thanks.

review: Approve

[Lucas Kanashiro (lucaskanashiro)]

Thanks for the review Sergio! Package uploaded:

$ dput ubuntu ../keepalived_2.0.19-2ubuntu0.2_source.changes
Checking signature on .changes
gpg: ../keepalived_2.0.19-2ubuntu0.2_source.changes: Valid signature from F823A2729883C97C
Checking signature on .dsc
gpg: ../keepalived_2.0.19-2ubuntu0.2.dsc: Valid signature from F823A2729883C97C
Package includes an .orig.tar.gz file although the debian revision suggests
that it might not be required. Multiple uploads of the .orig.tar.gz may be
rejected by the upload queue management software.
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading keepalived_2.0.19-2ubuntu0.2.dsc: done.
  Uploading keepalived_2.0.19.orig.tar.gz: done.
  Uploading keepalived_2.0.19-2ubuntu0.2.debian.tar.xz: done.
  Uploading keepalived_2.0.19-2ubuntu0.2_source.buildinfo: done.
  Uploading keepalived_2.0.19-2ubuntu0.2_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 653457e..c45a85a 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+keepalived (1:2.0.19-2ubuntu0.2) focal; urgency=medium
+
+  * Add patch fixing track_process with PID greater than 32767 (LP: #1960036)
+
+ -- Lucas Kanashiro <[e-mail]>  Mon, 07 Feb 2022 17:52:31 -0300
+
 keepalived (1:2.0.19-2ubuntu0.1) focal-security; urgency=medium
 
   * SECURITY UPDATE: Access-control bypass
diff --git a/debian/patches/Fix-track_process-with-PID-greater-than-32767.patch b/debian/patches/Fix-track_process-with-PID-greater-than-32767.patch
new file mode 100644
index 0000000..004021e
--- /dev/null
+++ b/debian/patches/Fix-track_process-with-PID-greater-than-32767.patch
@@ -0,0 +1,37 @@
+Description: Fix track_process with PIDs > 32767
+Origin: upstream, https://github.com/acassen/keepalived/commit/23a5b8113bf0b8
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/keepalived/+bug/1960036
+Reviewed-By: Lucas Kanashiro <[e-mail]>
+Last-Updated: 2022-02-07
+
+--- a/keepalived/core/track_process.c
++++ b/keepalived/core/track_process.c
+@@ -339,7 +339,7 @@
+ 	 * swapped in to read it. */
+ 	if (!have_comm) {
+ 		if (vrrp_data->vrrp_use_process_cmdline) {
+-			snprintf(cmdline, sizeof(cmdline), "/proc/%hd/cmdline", pid);
++			snprintf(cmdline, sizeof(cmdline), "/proc/%d/cmdline", pid);
+ 
+ 			if ((fd = open(cmdline, O_RDONLY)) == -1)
+ 				return;
+@@ -356,7 +356,7 @@
+ 		}
+ 
+ 		if (vrrp_data->vrrp_use_process_comm) {
+-			snprintf(cmdline, sizeof(cmdline), "/proc/%hd/comm", pid);
++			snprintf(cmdline, sizeof(cmdline), "/proc/%d/comm", pid);
+ 
+ 			fd = open(cmdline, O_RDONLY);
+ 			if (fd == -1) {
+--- a/lib/scheduler.c
++++ b/lib/scheduler.c
+@@ -440,7 +440,7 @@
+ 		prog_id = child_finder_name(pid);
+ 
+ 	if (!prog_id) {
+-		snprintf(pid_buf, sizeof(pid_buf), "pid %hd", pid);
++		snprintf(pid_buf, sizeof(pid_buf), "pid %d", pid);
+ 		prog_id = pid_buf;
+ 	}
+ 
diff --git a/debian/patches/series b/debian/patches/series
index c6c2d69..1874b77 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1 +1,2 @@
 CVE-2021-44225.patch
+Fix-track_process-with-PID-greater-than-32767.patch