View Issue Details

IDProjectCategoryView StatusLast Update
0000614Pgpool-IIBugpublic2020-05-27 16:41
Reportergregn123Assigned Tot-ishii 
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionopen 
Product Version4.1.0 
Target Version4.1.3Fixed in Version 
Summary0000614: Invalid memory reference in sync_backend_from_watchdog()
DescriptionThis refers to the following code in the sync_backend_from_watchdog() function, in src/main/pgpool_main.c:

        if (backendStatus->primary_node_id == -1 && BACKEND_INFO(Req_info->primary_node_id).backend_status != CON_DOWN)
        {
            ereport(LOG,
                (errmsg("primary node:%d on master watchdog node \"%s\" seems to be quarantined",
                    Req_info->primary_node_id, backendStatus->nodeName),
                errdetail("keeping the current primary")));
        }
        else
        {
            Req_info->primary_node_id = backendStatus->primary_node_id;
            primary_changed = true;
        }


During run of regression test 004.watchdog, sync_backend_from_watchdog() referenced BACKEND_INFO using a primary node id of -2, which caused memory to be referenced outside of the BACKEND_INFO.
On RHEL7 for IBM Z, this always caused a crash (coredump).

The problem was introduced by the following commit:
https://github.com/pgpool/pgpool2/commit/3922c12c1f8efbc1b5f2e7def1e0ff921aafb989
 
I've attached a patch for review.

Steps To Reproduceregression test 004.watchdog
The crash may or may not occur, depending upon whether the memory referenced by BACKEND_INFO(-2) is actually accessible or not.

We didn't get a crash when running under Linux on Intel hardware (obviously the memory it referenced was accessible, but the value retrieved was essentially junk).

Running under Linux on IBM Z hardware, it always crashed.
TagsNo tags attached.

Activities

gregn123

2020-05-26 16:07

reporter  

0001-Fix-invalid-memory-ref-in-sync_backend_from_watchdog.patch (1,477 bytes)
From 707cae53cc1d01268b05f25f4ae5ba628ce59bc8 Mon Sep 17 00:00:00 2001
From: Greg Nancarrow <gregn4422@gmail.com>
Date: Tue, 26 May 2020 14:31:35 +1000
Subject: [PATCH] Fix invalid memory reference in sync_backend_from_watchdog()

During run of regression test 004.watchdog, sync_backend_from_watchdog()
referenced BACKEND_INFO using a primary node id of -2, which caused
memory to be referenced outside of the BACKEND_INFO.
On RHEL7 for IBM Z, this caused a crash (coredump).
The problem was introduced by the following commit:
https://github.com/pgpool/pgpool2/commit/3922c12c1f8efbc1b5f2e7def1e0ff921aafb989
---
 src/main/pgpool_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/main/pgpool_main.c b/src/main/pgpool_main.c
index 8b1ea2a..1265ecf 100644
--- a/src/main/pgpool_main.c
+++ b/src/main/pgpool_main.c
@@ -3867,7 +3867,8 @@ sync_backend_from_watchdog(void)
 		 * So we will not update our primary node id when the status of current primary node
 		 * is not CON_DOWN while primary_node_id sent by master watchdong node is -1
 		 */
-		if (backendStatus->primary_node_id == -1 && BACKEND_INFO(Req_info->primary_node_id).backend_status != CON_DOWN)
+		if (Req_info->primary_node_id >= 0 &&
+			backendStatus->primary_node_id == -1 && BACKEND_INFO(Req_info->primary_node_id).backend_status != CON_DOWN)
 		{
 			ereport(LOG,
                 (errmsg("primary node:%d on master watchdog node \"%s\" seems to be quarantined",
-- 
1.8.3.1

t-ishii

2020-05-26 22:44

developer   ~0003370

Thank you for the detailed analysis and the patch. -2 is the initial value for Req_info->primary_node_id, that means that the variable was never be set by the time when the crash occurs. So question is, why it was not updated and left as an initial value -2? When the regression test is executed, pgpool is running in "raw mode", which assumes that PostgreSQL is not running in streaming replication mode. If pgpool runs in raw mode, Req_info->primary_node_id will be left the initial value and will never be updated. So I think the whole code block below should only be executed in streaming replication mode. This does not mean your patch is bogus: I think your patch should be applied as well to guard against the unwanted out of band array access (I guess that could happen depending on timing). I will work on this tomorrow.

if (Req_info->primary_node_id != backendStatus->primary_node_id)
    {
        ereport(LOG,
                (errmsg("Req_info->primary_node_id: %d processState: %d", Req_info->primary_node_id, processState)));

        /* Do not produce this log message if we are starting up the Pgpool-II */
        if (processState != INITIALIZING)
            ereport(LOG,
                    (errmsg("primary node:%d on master watchdog node \"%s\" is different from local primary node:%d",
                            backendStatus->primary_node_id, backendStatus->nodeName, Req_info->primary_node_id)));
        /*
         * master node returns primary_node_id = -1 when the node primary
         * node is in quarantine state on the master.
         * So we will not update our primary node id when the status of current primary node
         * is not CON_DOWN while primary_node_id sent by master watchdong node is -1
         */
        if (backendStatus->primary_node_id == -1 && BACKEND_INFO(Req_info->primary_node_id).backend_status != CON_DOWN)
        {
            ereport(LOG,
                (errmsg("primary node:%d on master watchdog node \"%s\" seems to be quarantined",
                    Req_info->primary_node_id, backendStatus->nodeName),
                errdetail("keeping the current primary")));
        }
        else
        {
            Req_info->primary_node_id = backendStatus->primary_node_id;
            primary_changed = true;
        }
    }

t-ishii

2020-05-27 15:43

developer   ~0003371

> I will work on this tomorrow.
Done. Patch committed with slight modifications by me.
Thank you for the patch!
https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=cd32f6ab91d77ccdb253da5da3e2e1b6f3c933bf

Issue History

Date Modified Username Field Change
2020-05-26 16:07 gregn123 New Issue
2020-05-26 16:07 gregn123 File Added: 0001-Fix-invalid-memory-ref-in-sync_backend_from_watchdog.patch
2020-05-26 16:17 t-ishii Assigned To => t-ishii
2020-05-26 16:17 t-ishii Status new => assigned
2020-05-26 16:17 t-ishii Description Updated View Revisions
2020-05-26 16:17 t-ishii Steps to Reproduce Updated View Revisions
2020-05-26 22:44 t-ishii Note Added: 0003370
2020-05-27 15:43 t-ishii Note Added: 0003371
2020-05-27 15:44 t-ishii Status assigned => resolved
2020-05-27 16:41 administrator Target Version => 4.1.3