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

[Christian Ehrhardt ]

[Christian Ehrhardt  (paelzer)]

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

[Andreas Hasenack (ahasenack)]

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

[Christian Ehrhardt  (paelzer)]

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?

[Christian Ehrhardt  (paelzer)]

tags pushed, and dput to Cosmic

diff --git a/debian/changelog b/debian/changelog
index 87d8bcd..00a1796 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+pgpool2 (3.7.3-1ubuntu1) cosmic; urgency=medium
+
+  * d/p/lp1777418-fix-arm-crash.patch: Fix crash on arm due to different
+    retval handling triggering a segfault (LP: #1777418).
+
+ -- Christian Ehrhardt <[e-mail]>  Mon, 18 Jun 2018 12:18:37 +0200
+
 pgpool2 (3.7.3-1) unstable; urgency=medium
 
   * New upstream version.
diff --git a/debian/patches/lp1777418-fix-arm-crash.patch b/debian/patches/lp1777418-fix-arm-crash.patch
new file mode 100644
index 0000000..4d4ca00
--- /dev/null
+++ b/debian/patches/lp1777418-fix-arm-crash.patch
@@ -0,0 +1,50 @@
+From 75b27e72537a5d0c8e1be2c9a405328accdc8133 Mon Sep 17 00:00:00 2001
+From: Tatsuo Ishii <[e-mail]>
+Date: Tue, 19 Jun 2018 10:10:33 +0900
+Subject: [PATCH] Fix segfault in per_node_error_log() on armhf architecture.
+
+pool_extract_error_message() incorrectly returns 255 (in decimal) on
+the architecture when previous message was not an error or a notice
+message. In this case per_node_error_log() happily calls ereport since
+the return value from pool_extract_error_message() is greater than
+0. Unfortunately the message string returned by
+pool_extract_error_message() points to garbage memory in this case, a
+segfault occurs.
+
+The fix gives per_node_error_log() a guard against the bug of
+pool_extract_error_message(). Moreover, the change is more consistent
+with other places where pool_extract_error_message() is called.
+
+Fix for pool_extract_error_message() will come later on.
+
+See:
+https://github.com/pgpool/pgpool2/issues/14
+for more detailed discussion.
+
+Problem reported and patch by Christian Ehrhardt.
+
+Author: Christian Ehrhardt <[e-mail]>
+Original-Author: Tatsuo Ishii <[e-mail]>
+Origin: backport, https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=75b27e72537a5d0c8e1be2c9a405328accdc8133
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1777418
+Last-Update: 2018-06-19
+---
+ src/protocol/pool_proto_modules.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/src/protocol/pool_proto_modules.c b/src/protocol/pool_proto_modules.c
+index f8a435e9..91e37aa0 100644
+--- a/src/protocol/pool_proto_modules.c
++++ b/src/protocol/pool_proto_modules.c
+@@ -3139,7 +3139,7 @@ void per_node_error_log(POOL_CONNECTION_POOL *backend, int node_id, char *query,
+ 	POOL_CONNECTION_POOL_SLOT *slot = backend->slots[node_id];
+ 	char *message;
+ 
+-	if (pool_extract_error_message(true, CONNECTION(backend, node_id), MAJOR(backend), true, &message) > 0)
++	if (pool_extract_error_message(true, CONNECTION(backend, node_id), MAJOR(backend), true, &message) == 1)
+ 	{
+ 		ereport(LOG,
+ 			(errmsg("%s: DB node id: %d backend pid: %d statement: \"%s\" message: \"%s\"",
+-- 
+2.17.1
+
diff --git a/debian/patches/series b/debian/patches/series
index c0c8554..9ec4175 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
 pgpool2-debian-config.patch
 sbin-paths
+lp1777418-fix-arm-crash.patch