Merge ~paelzer/ubuntu/+source/pgpool2:lp1777418-fix-arm-crash into ubuntu/+source/pgpool2:ubuntu/cosmic-devel

Proposed by Christian Ehrhardt 
Status: Merged
Merge reported by: Christian Ehrhardt 
Merged at revision: c69e4f4ac587debfaecc8c9d41315d26a424ca8d
Proposed branch: ~paelzer/ubuntu/+source/pgpool2:lp1777418-fix-arm-crash
Merge into: ubuntu/+source/pgpool2:ubuntu/cosmic-devel
Diff against target: 79 lines (+58/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/lp1777418-fix-arm-crash.patch (+50/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack (community) Approve
Canonical Server Team Pending
Ubuntu Server Dev import team Pending
Review via email: mp+348198@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Upstream discussion and inclusion [1]
Test build [2] in ppa.

This is already applied to all stable branches and master, so we can assume that we can sync over it on any next debian version.

[1]: https://github.com/pgpool/pgpool2/issues/14
[2]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3296

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

Looks good, +1. Great work on the analysis, specially on a non-traditional arch. I started this review by preparing my raspberrypi3 but will just give up, so slow it is :)

One comment only, entirely optional. It's just that I have been given this kind of review feedback in the past about patches. It was suggested that they be "clean" without the equivalent of --show-c-functions bits, i.e.:

+@@ -3139,7 +3139,7 @@

instead of

+@@ -3139,7 +3139,7 @@ void per_node_error_log(POOL_CONNECTION_POOL *backend, int node_id, char *query,

One way to do it, assuming you have ~/.quiltrc & friends setup up like that, is to just refresh it:
dquilt push lp1777418-fix-arm-crash.patch
dquilt refresh lp1777418-fix-arm-crash.patch
dquilt pop -a

Just a comment, +1 without that.

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

I actually got Debian feedback twice that I should have those extra context in the diff.
Due to that my dquilt .quiltrc has --show-c-function :-)
And I personally like it as well.
If I'd refresh with my quiltrc it might shorten it, but keep the functions.

Unless we have a rule (that I'd want to challenge then) I'll keep the function in the diff hunk headers.

Thanks for the review, just curious what was your source of "you should not have these" feedback?

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

tags pushed, and dput to Cosmic

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 87d8bcd..00a1796 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+pgpool2 (3.7.3-1ubuntu1) cosmic; urgency=medium
7+
8+ * d/p/lp1777418-fix-arm-crash.patch: Fix crash on arm due to different
9+ retval handling triggering a segfault (LP: #1777418).
10+
11+ -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Mon, 18 Jun 2018 12:18:37 +0200
12+
13 pgpool2 (3.7.3-1) unstable; urgency=medium
14
15 * New upstream version.
16diff --git a/debian/patches/lp1777418-fix-arm-crash.patch b/debian/patches/lp1777418-fix-arm-crash.patch
17new file mode 100644
18index 0000000..4d4ca00
19--- /dev/null
20+++ b/debian/patches/lp1777418-fix-arm-crash.patch
21@@ -0,0 +1,50 @@
22+From 75b27e72537a5d0c8e1be2c9a405328accdc8133 Mon Sep 17 00:00:00 2001
23+From: Tatsuo Ishii <ishii@postgresql.org>
24+Date: Tue, 19 Jun 2018 10:10:33 +0900
25+Subject: [PATCH] Fix segfault in per_node_error_log() on armhf architecture.
26+
27+pool_extract_error_message() incorrectly returns 255 (in decimal) on
28+the architecture when previous message was not an error or a notice
29+message. In this case per_node_error_log() happily calls ereport since
30+the return value from pool_extract_error_message() is greater than
31+0. Unfortunately the message string returned by
32+pool_extract_error_message() points to garbage memory in this case, a
33+segfault occurs.
34+
35+The fix gives per_node_error_log() a guard against the bug of
36+pool_extract_error_message(). Moreover, the change is more consistent
37+with other places where pool_extract_error_message() is called.
38+
39+Fix for pool_extract_error_message() will come later on.
40+
41+See:
42+https://github.com/pgpool/pgpool2/issues/14
43+for more detailed discussion.
44+
45+Problem reported and patch by Christian Ehrhardt.
46+
47+Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
48+Original-Author: Tatsuo Ishii <ishii@postgresql.org>
49+Origin: backport, https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=75b27e72537a5d0c8e1be2c9a405328accdc8133
50+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1777418
51+Last-Update: 2018-06-19
52+---
53+ src/protocol/pool_proto_modules.c | 2 +-
54+ 1 file changed, 1 insertion(+), 1 deletion(-)
55+
56+diff --git a/src/protocol/pool_proto_modules.c b/src/protocol/pool_proto_modules.c
57+index f8a435e9..91e37aa0 100644
58+--- a/src/protocol/pool_proto_modules.c
59++++ b/src/protocol/pool_proto_modules.c
60+@@ -3139,7 +3139,7 @@ void per_node_error_log(POOL_CONNECTION_POOL *backend, int node_id, char *query,
61+ POOL_CONNECTION_POOL_SLOT *slot = backend->slots[node_id];
62+ char *message;
63+
64+- if (pool_extract_error_message(true, CONNECTION(backend, node_id), MAJOR(backend), true, &message) > 0)
65++ if (pool_extract_error_message(true, CONNECTION(backend, node_id), MAJOR(backend), true, &message) == 1)
66+ {
67+ ereport(LOG,
68+ (errmsg("%s: DB node id: %d backend pid: %d statement: \"%s\" message: \"%s\"",
69+--
70+2.17.1
71+
72diff --git a/debian/patches/series b/debian/patches/series
73index c0c8554..9ec4175 100644
74--- a/debian/patches/series
75+++ b/debian/patches/series
76@@ -1,2 +1,3 @@
77 pgpool2-debian-config.patch
78 sbin-paths
79+lp1777418-fix-arm-crash.patch

Subscribers

People subscribed via source and target branches