From be6897ce6375e9bcb39e0d25be5b1cbd450630e3 Mon Sep 17 00:00:00 2001 From: Annpurna Shahani Date: Tue, 26 Sep 2023 17:21:59 +0800 Subject: [PATCH] [7X] gpconfig does not escape '$' char Issue: gpconfig is not able to set the guc correctly if the value contains '$'. It considers the string as a shell var and evaluates it, instead of escaping the '$' character [~/workspace/gpdb/gpMgmt: {main} ?]$ gpconfig --verbose -c dynamic_library_path -v '$libdir:/foo/bar' 20230906:11:33:38:023885 gpconfig:-[INFO]:-completed successfully with parameters '--verbose -c dynamic_library_path -v '$libdir:/foo/bar'' [~/workspace/gpdb/gpMgmt: {main} ?]$ gpstop -u 20230906:11:35:17:024203 gpstop:xx-[INFO]:-Greenplum Version: 'postgres (Greenplum Database) 7.0.0-beta.4+dev.254.g1621aed51f build dev' 20230906:11:35:17:024203 gpstop:xx-[INFO]:-Signalling all postmaster processes to reload [~/workspace/gpdb/gpMgmt: {main} ?]$ gpconfig -s dynamic_library_path Values on all segments are consistent GUC : dynamic_library_path Coordinator value: :/foo/bar Segment value: :/foo/bar Reason: gpconfig does not encode the guc value when invoking the gpconfig_helper script to add the guc parameter to postgressql.conf. Just quotes the value using shlex.quote().Value is sent as qouted plain text hence the $ char is not escaped and the value string is considered as shell variable. shlex.quote() is not efficient enough to add escape char ('/') before '$' in guc value. This issue does not exist in 6X as value is encoded using base64.urlsafe_b64encode() then sent to segment hosts. Fix: Encoding the value at coordinator and then decoding it on segment host will make sure that value string is intact. Reverted the gpconfig changes done in commit 50b06308db1e3f0b4e310c25f4501e6f11884d6a which introduced the use of shlex.quote() to qoute the guc values instead of encoding it. After fix: [~/workspace/gpdb/gpMgmt: {gpconfig-issue} ?]$ gpconfig -c dynamic_library_path -v '$libdir:/foo/bar' 20230915:16:55:55:054429 gpconfig:-[INFO]:-completed successfully with parameters '-c dynamic_library_path -v '$libdir:/foo/bar'' [~/workspace/gpdb/gpMgmt: {gpconfig-issue} ?]$ gpstop -u 20230915:16:56:11:054645 gpstop:-[INFO]:-Greenplum Version: 'postgres (Greenplum Database) 7.0.0-beta.4+dev.254.g1621aed51f build dev' 20230915:16:56:11:054645 gpstop:-[INFO]:-Signalling all postmaster processes to reload [~/workspace/gpdb/gpMgmt: {gpconfig-issue} ?]$ gpconfig -s dynamic_library_path Values on all segments are consistent GUC : dynamic_library_path Coordinator value: $libdir:/foo/bar Segment value: $libdir:/foo/bar (cherry picked from commit 9383cb7612dbd7e2e05100e85e0ac47c763fa03c) --- gpMgmt/bin/gppylib/commands/gp.py | 7 +++++-- .../bin/gppylib/test/unit/test_unit_gpconfig.py | 16 ++++++++-------- gpMgmt/sbin/gpconfig_helper.py | 6 ++++-- gpMgmt/test/behave/mgmt_utils/gpconfig.feature | 1 + 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/gpMgmt/bin/gppylib/commands/gp.py b/gpMgmt/bin/gppylib/commands/gp.py index 21ae6df7a45..46d2f636b7d 100644 --- a/gpMgmt/bin/gppylib/commands/gp.py +++ b/gpMgmt/bin/gppylib/commands/gp.py @@ -8,6 +8,8 @@ """ import json import os, time +import base64 +import pickle import shlex import os.path import pipes @@ -1101,7 +1103,7 @@ def __init__(self, command_name, postgresconf_dir, name, value=None, segInfo=Non addParameter = (not getParameter) and (not removeParameter) if addParameter: - args = "--add-parameter %s --value %s " % (name, shlex.quote(value)) + args = "--add-parameter %s --value %s " % (name, base64.urlsafe_b64encode(pickle.dumps(value)).decode()) if getParameter: args = "--get-parameter %s" % name if removeParameter: @@ -1115,7 +1117,8 @@ def __init__(self, command_name, postgresconf_dir, name, value=None, segInfo=Non # FIXME: figure out how callers of this can handle exceptions here def get_value(self): - return self.get_results().stdout + raw_value = self.get_results().stdout + return pickle.loads(base64.urlsafe_b64decode(raw_value)) #----------------------------------------------- diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py b/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py index 298cf80d61c..54fa8f3cf7c 100644 --- a/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py +++ b/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py @@ -1,7 +1,7 @@ import errno import imp import os -import shlex +import base64, pickle import shutil import sys import tempfile @@ -289,11 +289,11 @@ def test_option_change_value_coordinator_separate_succeed(self): self.assertEqual(self.pool.addCommand.call_count, 5) segment_command = self.pool.addCommand.call_args_list[0][0][0] self.assertTrue("my_property_name" in segment_command.cmdStr) - value = shlex.quote("100") + value = base64.urlsafe_b64encode(pickle.dumps("100")).decode() self.assertTrue(value in segment_command.cmdStr) coordinator_command = self.pool.addCommand.call_args_list[4][0][0] self.assertTrue("my_property_name" in coordinator_command.cmdStr) - value = shlex.quote("20") + value = base64.urlsafe_b64encode(pickle.dumps("20")).decode() self.assertTrue(value in coordinator_command.cmdStr) def test_option_change_value_coordinatoronly_succeed(self): @@ -312,7 +312,7 @@ def test_option_change_value_coordinatoronly_succeed(self): self.assertEqual(self.pool.addCommand.call_count, 1) coordinator_command = self.pool.addCommand.call_args_list[0][0][0] self.assertTrue(("my_property_name") in coordinator_command.cmdStr) - value = shlex.quote("100") + value = base64.urlsafe_b64encode(pickle.dumps("100")).decode() self.assertTrue(value in coordinator_command.cmdStr) def test_new_option_change_value_coordinatoronly_succeed(self): @@ -331,7 +331,7 @@ def test_new_option_change_value_coordinatoronly_succeed(self): self.assertEqual(self.pool.addCommand.call_count, 1) coordinator_command = self.pool.addCommand.call_args_list[0][0][0] self.assertTrue(("my_property_name") in coordinator_command.cmdStr) - value = shlex.quote("100") + value = base64.urlsafe_b64encode(pickle.dumps("100")).decode() self.assertTrue(value in coordinator_command.cmdStr) def test_old_and_new_option_change_value_coordinatoronly_succeed(self): @@ -350,7 +350,7 @@ def test_old_and_new_option_change_value_coordinatoronly_succeed(self): self.assertEqual(self.pool.addCommand.call_count, 1) coordinator_command = self.pool.addCommand.call_args_list[0][0][0] self.assertTrue(("my_property_name") in coordinator_command.cmdStr) - value = shlex.quote("100") + value = base64.urlsafe_b64encode(pickle.dumps("100")).decode() self.assertTrue(value in coordinator_command.cmdStr) @@ -373,7 +373,7 @@ def test_option_change_value_hidden_guc_with_skipvalidation(self): self.assertTrue("my_hidden_guc_name" in segment_command.cmdStr) coordinator_command = self.pool.addCommand.call_args_list[1][0][0] self.assertTrue("my_hidden_guc_name" in coordinator_command.cmdStr) - value = shlex.quote("100") + value = base64.urlsafe_b64encode(pickle.dumps("100")).decode() self.assertTrue(value in coordinator_command.cmdStr) def test_option_change_value_hidden_guc_without_skipvalidation(self): @@ -579,7 +579,7 @@ def validation_for_testing_quoting_string_values(self, expected_value): # In this case, we have an object as an argument to poo.addCommand # call_obj[1] returns a dict for all named arguments -> {key='arg3', key2='arg4'} gp_add_config_script_obj = call[0][0] - value = shlex.quote(expected_value) + value = base64.urlsafe_b64encode(pickle.dumps(expected_value)).decode() try: self.assertTrue(value in gp_add_config_script_obj.cmdStr) except AssertionError as e: diff --git a/gpMgmt/sbin/gpconfig_helper.py b/gpMgmt/sbin/gpconfig_helper.py index 3191957c651..158e0332cb4 100755 --- a/gpMgmt/sbin/gpconfig_helper.py +++ b/gpMgmt/sbin/gpconfig_helper.py @@ -16,6 +16,8 @@ import shutil import sys import tempfile + import base64 + import pickle from optparse import Option, OptionParser from gppylib.gpparseopts import OptParser, OptChecker @@ -101,7 +103,7 @@ def add_parameter(filename, name, value): for line in lines: outfile.write(line) new_lines = new_lines + 1 - outfile.write(name + '=' + value + os.linesep) + outfile.write(name + '=' + pickle.loads(base64.urlsafe_b64decode(value)) + os.linesep) new_lines = new_lines + 1 if new_lines == len(lines) + 1: @@ -122,7 +124,7 @@ def main(): if options.get_parameter: try: value = get_parameter(options.file, options.get_parameter) - sys.stdout.write(value) + sys.stdout.write(base64.urlsafe_b64encode(pickle.dumps(value)).decode()) return except Exception as err: sys.stderr.write("Failed to get value for parameter '%s' in file %s due to: %s" % ( diff --git a/gpMgmt/test/behave/mgmt_utils/gpconfig.feature b/gpMgmt/test/behave/mgmt_utils/gpconfig.feature index 31c6478e633..04715babb0d 100644 --- a/gpMgmt/test/behave/mgmt_utils/gpconfig.feature +++ b/gpMgmt/test/behave/mgmt_utils/gpconfig.feature @@ -74,6 +74,7 @@ Feature: gpconfig integration tests | float | checkpoint_completion_target | float | 0.4 | 0.5 | 0.5 | 0.5 | 0.33 | 0.33 | 0.7 | 0.7 | 0.7 | | basic string | application_name | string | xxxxxx | bodhi | 'bodhi' | bodhi | lucy | 'lucy' | bengie | 'bengie' | bengie | | string with spaces | application_name | string | yyyyyy | 'bod hi' | 'bod hi' | bod hi | 'bod hi' | 'bod hi' | 'bod hi' | 'bod hi' | bod hi | + | string with special characters | application_name | string | yyyyyy | '$li@d#r' | '$li@d#r' | $li@d#r | '$li@d#r' | '$li@d#r' | '$li@d#r' | '$li@d#r' | $li@d#r | | different value on coordinator and segments | application_name | string | yyyyyy | 'bod hi' | 'bod hi' | bod hi | 'lu cy' | 'lu cy' | 'ben gie' | 'ben gie' | ben gie | | empty string | application_name | string | zzzzzz | '' | '' | | '' | '' | '' | '' | | | quoted double quotes | application_name | string | zzzzzz | '"hi"' | '"hi"' | "hi" | '"hi"' | '"hi"' | '"hi"' | '"hi"' | "hi" |