From df5460eab159b7cdc3528337369433bac72e1b50 Mon Sep 17 00:00:00 2001 From: Shoaib Lari Date: Thu, 30 Mar 2017 16:31:49 -0700 Subject: [PATCH] Fixing Coverity errors for FTS component. This commit fixes the following Coverity errors. ftsfilerep.c: 129923 (for fts.c), 160618, 160637, 160639 ftsprobe.c: 129898 primary_mirror_mode.c: 160714 (for ftsprobe.c) ftssan.c: 160620, 160636, 160638, 157330 The individual Coverity warnings are described below ftsfilerep.c ============ CID 129923 ---------- The Coverity error is reported in fts.c. However, the fix is in ftsfilerep.c. In function probePublishUpdate(), function FtsGetPairStateFilerep() will return FILEREP_SENTINEL if the segements are in inconsistent state. This value is passed to transition() as the stateOld parameter. The function transition() calls FtsTransitionFilerep() with this value of stateOld. FtsTransitionFilerep() uses stateOld to index the state_machine_filerep array. However, this causes the following warning from Coverity. Out-of-bounds access (OVERRUN) overrun-call: Overrunning callee's array of size 3 by passing argument stateOld (which evaluates to 4) in call to transition. There is an Assert in FtsTransitionFilerep() to check the validity of stateOld. However, production code is compiled without Assert, so this is a possible error condition at runtime. The fix is to log an error when an invalid stateOld value is passed to FtsTranstionFilerep() and return without indexing the array. CID 160618 ---------- Added missing break in FtsResolveStateFilerep() for FILEREP_PUS_MUS and FILEREP_PUR_MUR cases. CID 160637 ---------- The IS_VALID_OLD_STATE_FILEREP(state) macro is defined as: (state >= FILEREP_PUS_MUS && state <= FILEREP_PUC_MDX) where the FILEREP_* states are enums starting at 0. The macro is called with state defined as uint32. This results in the following Coverity warning. Macro compares unsigned to 0 (NO_EFFECT) unsigned_compare: This greater-than-or-equal-to-zero comparison of an unsigned value is always true. stateOld >= FILEREP_PUS_MUS I have modified the macro definition to: ((unsigned int)(state) <= FILEREP_PUC_MDX) 160639 ------ The IS_VALID_NEW_STATE_FILEREP(state) macro is defined as: (state >= FILEREP_PUS_MUS && state < FILEREP_SENTINEL) where the FILEREP_* states are enums starting at 0. The macro is called with state defined as uint32. This results in the following Coverity warning. Macro compares unsigned to 0 (NO_EFFECT) unsigned_compare: This greater-than-or-equal-to-zero comparison of an unsigned value is always true. pairState->stateNew >= FILEREP_PUS_MUS. I have modified the macro definition to: ((unsigned int)(state) < FILEREP_SENTINEL) ftsprobe.c ========== CID 129898 ---------- In function probeTimeout(), we have: unint64 elapsed_ms = ...; if (elapsed_ms > gp_fts_probe_timeout * 1000) ... gp_fts_probe_timout is defined as an int. Therefore, we get the following Coverity error: Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN) overflow_before_widen: Potentially overflowing expression gp_fts_probe_timeout * 1000 with type int (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type uint64 (64 bits, unsigned). I have casted gp_fts_probe_timout to uint64 to resolve this. primary_mirror_mode.c ===================== CID 160714 ---------- Function probeProcessResponse() in ftsprobe.c reads a network packet to process probe response from the segments. The fault field from the packet is passed to getFaultType() in primary_mirror_mode.c to index the labels array. Therefore, we get the following warning from Coverity. Use of untrusted scalar value (TAINTED_SCALAR) tainted_data: Passing tainted variable fault to a tainted sink. I resolved this my making sure that the faultType is within the dimension of the labels array before using the faultType as an index. ftssan.c ========= CID 160620 ---------- Added missing break in FtsResolveStateSAN() for the SAN_PU_MU case. CID 160636 ---------- The IS_VALID_NEW_STATE_SN(state) macro is defined as: (state >= SAN_PU_MU && state < SAN_SENTINEL) where the SAN_* states are enums starting at 0. The macro is called with state defined as uint32. This results in the following Coverity warning. Macro compares unsigned to 0 (NO_EFFECT) unsigned_compare: This greater-than-or-equal-to-zero comparison of an unsigned value is always true. pairState->stateNew >= SAN_PU_MU. I have modified the macro definition to: ((unsigned int)(state) < SAN_SENTINEL) CID 160638 ----------- The IS_VALID_OLD_STATE_SAN(state) macro is defined as: (state >= SAN_PU_MU && state <= SAN_PU_MD) where the SAN_* states are enums starting at 0. The macro is called with state defined as uint32. This results in the following Coverity warning. Macro compares unsigned to 0 (NO_EFFECT) unsigned_compare: This greater-than-or-equal-to-zero comparison of an unsigned value is always true. stateOld >= SAN_PU_MU. I have modified the macro definition to: ((unsigned int)(state) <= SAN_PU_MD) CID 157330 ---------- The mountMaintenanceForMpid() function sets the gphome_env variable to getenv("GPHOME"). The gphome_env variable is used as a file path in a command string that is passed to the system() OS call. This results in the following warning from Coverity. Use of untrusted string value (TAINTED_STRING) tainted_string: Passing tainted string cmd to system, which cannot accept tainted data. I resolved this by using the execlp() system call instead of system() as recommended in https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=2130132. To use execlp(), individual arguments have to be built as separate characters strings. --- src/backend/fts/ftsfilerep.c | 14 ++++--- src/backend/fts/ftsprobe.c | 2 +- src/backend/fts/ftssan.c | 41 +++++++++++++------- src/backend/postmaster/primary_mirror_mode.c | 5 ++- 4 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/backend/fts/ftsfilerep.c b/src/backend/fts/ftsfilerep.c index 2db67eb1a7..c1414fdb35 100644 --- a/src/backend/fts/ftsfilerep.c +++ b/src/backend/fts/ftsfilerep.c @@ -53,10 +53,10 @@ enum filerep_segment_pair_state_e /* we always assume that primary is up */ #define IS_VALID_OLD_STATE_FILEREP(state) \ - (state >= FILEREP_PUS_MUS && state <= FILEREP_PUC_MDX) + ((unsigned int)(state) <= FILEREP_PUC_MDX) #define IS_VALID_NEW_STATE_FILEREP(state) \ - (state >= FILEREP_PUS_MUS && state < FILEREP_SENTINEL) + ((unsigned int)(state) < FILEREP_SENTINEL) /* * state machine matrix for filerep; @@ -171,15 +171,17 @@ FtsGetPairStateFilerep(CdbComponentDatabaseInfo *primary, CdbComponentDatabaseIn /* - * get new state for primary and mirror using filerep state machine + * Get new state for primary and mirror using filerep state machine. + * In case of an invalid old state, log the old state and do not transition. */ uint32 FtsTransitionFilerep(uint32 stateOld, uint32 trans) { - Assert(IS_VALID_OLD_STATE_FILEREP(stateOld)); - int i = 0; + if (!(IS_VALID_OLD_STATE_FILEREP(stateOld))) + elog(ERROR, "FTS: invalid old state for transition: %d", stateOld); + /* check state machine for transition */ for (i = 0; i < FILEREP_SENTINEL; i++) { @@ -253,6 +255,8 @@ FtsResolveStateFilerep(FtsSegmentPairState *pairState) case (FILEREP_PUS_MUS): case (FILEREP_PUR_MUR): Assert(!"FTS is not responsible for bringing segments back to life"); + break; + default: Assert(!"Invalid transition in filerep state machine"); } diff --git a/src/backend/fts/ftsprobe.c b/src/backend/fts/ftsprobe.c index cc2cc37a7e..ddf44a9f41 100644 --- a/src/backend/fts/ftsprobe.c +++ b/src/backend/fts/ftsprobe.c @@ -676,7 +676,7 @@ probeTimeout(ProbeConnectionInfo *probeInfo, const char* calledFrom) } /* If connection takes more than the gp_fts_probe_timeout, we fail. */ - if (elapsed_ms > gp_fts_probe_timeout * 1000) + if (elapsed_ms > (uint64)gp_fts_probe_timeout * 1000) { write_log("FTS: failed to probe segment (content=%d, dbid=%d) due to timeout expiration, " "probe elapsed time: " UINT64_FORMAT " ms.", diff --git a/src/backend/fts/ftssan.c b/src/backend/fts/ftssan.c index 6fb533437f..bbb1084260 100644 --- a/src/backend/fts/ftssan.c +++ b/src/backend/fts/ftssan.c @@ -33,6 +33,9 @@ #include #endif +#include + + /* * CONSTANTS */ @@ -67,10 +70,10 @@ enum san_segment_pair_state_e }; #define IS_VALID_OLD_STATE_SAN(state) \ - (state >= SAN_PU_MU && state <= SAN_PU_MD) + ((unsigned int)(state) <= SAN_PU_MD) #define IS_VALID_NEW_STATE_SAN(state) \ - (state >= SAN_PU_MU && state < SAN_SENTINEL) + ((unsigned int)(state) < SAN_SENTINEL) /* * state machine matrix for SAN; @@ -201,6 +204,8 @@ FtsResolveStateSAN(FtsSegmentPairState *pairState) case (SAN_PU_MU): Assert(!"FTS is not responsible for bringing segments back to life"); + break; + default: Assert(!"Invalid transition in filerep state machine"); } @@ -555,6 +560,7 @@ mountMaintenanceForMpid(int mpid) { char *gphome_env=NULL; char cmd[1024]; + char args[8][256]; int cmd_status; /* @@ -569,19 +575,26 @@ mountMaintenanceForMpid(int mpid) gphome_env = getenv("GPHOME"); if (gphome_env == NULL) - { - snprintf(cmd, sizeof(cmd), "gp_mount_agent --agent -t %c -a %c -p %s -d %s -m %s -q %s -e %s -n %s", - san_type, active_host, p_host, p_dev, p_mp, m_host, m_dev, m_mp); - } + snprintf(cmd, sizeof(cmd), "gp_mount_agent"); else - { - snprintf(cmd, sizeof(cmd), "%s/bin/gp_mount_agent --agent -t %c -a %c -p %s -d %s -m %s -q %s -e %s -n %s", - gphome_env, san_type, active_host, p_host, p_dev, p_mp, m_host, m_dev, m_mp); - } - - elog(LOG, "Mount agent command is [%s]", cmd); - - cmd_status = system(cmd); + snprintf(cmd, sizeof(cmd), "%s/bin/gp_mount_agent", gphome_env); + + snprintf(args[0], sizeof(args[0]), "--agent -t %c", san_type); + snprintf(args[1], sizeof(args[1]), "-a %c", active_host); + snprintf(args[2], sizeof(args[2]), "-p %s", p_host); + snprintf(args[3], sizeof(args[3]), "-d %s", p_dev); + snprintf(args[4], sizeof(args[4]), "-m %s", p_mp); + snprintf(args[5], sizeof(args[5]), "-q %s", m_host); + snprintf(args[6], sizeof(args[6]), "-e %s", m_dev); + snprintf(args[7], sizeof(args[7]), "-n %s", m_mp); + + elog(LOG, "Mount agent command is [%s %s %s %s %s %s %s %s %s]", + cmd, args[0], args[1], args[2], args[3], args[4], args[5], + args[6], args[7]); + + cmd_status = execlp(cmd, "gp_mount_agent", args[0], args[1], + args[2], args[3], args[4], args[5], args[6], + args[7], NULL); if (cmd_status == -1) { diff --git a/src/backend/postmaster/primary_mirror_mode.c b/src/backend/postmaster/primary_mirror_mode.c index a737d513ba..da06387c73 100644 --- a/src/backend/postmaster/primary_mirror_mode.c +++ b/src/backend/postmaster/primary_mirror_mode.c @@ -373,7 +373,10 @@ getFaultTypeLabel(FaultType_e faultType) }; COMPILE_ASSERT(ARRAY_SIZE(labels) == FaultType__EnumerationCount); - return labels[faultType]; + if (faultType < ARRAY_SIZE(labels)) + return labels[faultType]; + else + return "UnrecognizedFaultType"; } /** -- GitLab