Skip to content

Commit be6897c

Browse files
Annu149my-ship-it
Annu149
authored and
my-ship-it
committed
[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)
1 parent 2e16543 commit be6897c

4 files changed

Lines changed: 18 additions & 12 deletions

File tree

‎gpMgmt/bin/gppylib/commands/gp.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
"""
99
import json
1010
import os, time
11+
import base64
12+
import pickle
1113
import shlex
1214
import os.path
1315
import pipes
@@ -1101,7 +1103,7 @@ def __init__(self, command_name, postgresconf_dir, name, value=None, segInfo=Non
11011103

11021104
addParameter = (not getParameter) and (not removeParameter)
11031105
if addParameter:
1104-
args = "--add-parameter %s --value %s " % (name, shlex.quote(value))
1106+
args = "--add-parameter %s --value %s " % (name, base64.urlsafe_b64encode(pickle.dumps(value)).decode())
11051107
if getParameter:
11061108
args = "--get-parameter %s" % name
11071109
if removeParameter:
@@ -1115,7 +1117,8 @@ def __init__(self, command_name, postgresconf_dir, name, value=None, segInfo=Non
11151117

11161118
# FIXME: figure out how callers of this can handle exceptions here
11171119
def get_value(self):
1118-
return self.get_results().stdout
1120+
raw_value = self.get_results().stdout
1121+
return pickle.loads(base64.urlsafe_b64decode(raw_value))
11191122

11201123

11211124
#-----------------------------------------------

‎gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import errno
22
import imp
33
import os
4-
import shlex
4+
import base64, pickle
55
import shutil
66
import sys
77
import tempfile
@@ -289,11 +289,11 @@ def test_option_change_value_coordinator_separate_succeed(self):
289289
self.assertEqual(self.pool.addCommand.call_count, 5)
290290
segment_command = self.pool.addCommand.call_args_list[0][0][0]
291291
self.assertTrue("my_property_name" in segment_command.cmdStr)
292-
value = shlex.quote("100")
292+
value = base64.urlsafe_b64encode(pickle.dumps("100")).decode()
293293
self.assertTrue(value in segment_command.cmdStr)
294294
coordinator_command = self.pool.addCommand.call_args_list[4][0][0]
295295
self.assertTrue("my_property_name" in coordinator_command.cmdStr)
296-
value = shlex.quote("20")
296+
value = base64.urlsafe_b64encode(pickle.dumps("20")).decode()
297297
self.assertTrue(value in coordinator_command.cmdStr)
298298

299299
def test_option_change_value_coordinatoronly_succeed(self):
@@ -312,7 +312,7 @@ def test_option_change_value_coordinatoronly_succeed(self):
312312
self.assertEqual(self.pool.addCommand.call_count, 1)
313313
coordinator_command = self.pool.addCommand.call_args_list[0][0][0]
314314
self.assertTrue(("my_property_name") in coordinator_command.cmdStr)
315-
value = shlex.quote("100")
315+
value = base64.urlsafe_b64encode(pickle.dumps("100")).decode()
316316
self.assertTrue(value in coordinator_command.cmdStr)
317317

318318
def test_new_option_change_value_coordinatoronly_succeed(self):
@@ -331,7 +331,7 @@ def test_new_option_change_value_coordinatoronly_succeed(self):
331331
self.assertEqual(self.pool.addCommand.call_count, 1)
332332
coordinator_command = self.pool.addCommand.call_args_list[0][0][0]
333333
self.assertTrue(("my_property_name") in coordinator_command.cmdStr)
334-
value = shlex.quote("100")
334+
value = base64.urlsafe_b64encode(pickle.dumps("100")).decode()
335335
self.assertTrue(value in coordinator_command.cmdStr)
336336

337337
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):
350350
self.assertEqual(self.pool.addCommand.call_count, 1)
351351
coordinator_command = self.pool.addCommand.call_args_list[0][0][0]
352352
self.assertTrue(("my_property_name") in coordinator_command.cmdStr)
353-
value = shlex.quote("100")
353+
value = base64.urlsafe_b64encode(pickle.dumps("100")).decode()
354354
self.assertTrue(value in coordinator_command.cmdStr)
355355

356356

@@ -373,7 +373,7 @@ def test_option_change_value_hidden_guc_with_skipvalidation(self):
373373
self.assertTrue("my_hidden_guc_name" in segment_command.cmdStr)
374374
coordinator_command = self.pool.addCommand.call_args_list[1][0][0]
375375
self.assertTrue("my_hidden_guc_name" in coordinator_command.cmdStr)
376-
value = shlex.quote("100")
376+
value = base64.urlsafe_b64encode(pickle.dumps("100")).decode()
377377
self.assertTrue(value in coordinator_command.cmdStr)
378378

379379
def test_option_change_value_hidden_guc_without_skipvalidation(self):
@@ -579,7 +579,7 @@ def validation_for_testing_quoting_string_values(self, expected_value):
579579
# In this case, we have an object as an argument to poo.addCommand
580580
# call_obj[1] returns a dict for all named arguments -> {key='arg3', key2='arg4'}
581581
gp_add_config_script_obj = call[0][0]
582-
value = shlex.quote(expected_value)
582+
value = base64.urlsafe_b64encode(pickle.dumps(expected_value)).decode()
583583
try:
584584
self.assertTrue(value in gp_add_config_script_obj.cmdStr)
585585
except AssertionError as e:

‎gpMgmt/sbin/gpconfig_helper.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
import shutil
1717
import sys
1818
import tempfile
19+
import base64
20+
import pickle
1921

2022
from optparse import Option, OptionParser
2123
from gppylib.gpparseopts import OptParser, OptChecker
@@ -101,7 +103,7 @@ def add_parameter(filename, name, value):
101103
for line in lines:
102104
outfile.write(line)
103105
new_lines = new_lines + 1
104-
outfile.write(name + '=' + value + os.linesep)
106+
outfile.write(name + '=' + pickle.loads(base64.urlsafe_b64decode(value)) + os.linesep)
105107
new_lines = new_lines + 1
106108

107109
if new_lines == len(lines) + 1:
@@ -122,7 +124,7 @@ def main():
122124
if options.get_parameter:
123125
try:
124126
value = get_parameter(options.file, options.get_parameter)
125-
sys.stdout.write(value)
127+
sys.stdout.write(base64.urlsafe_b64encode(pickle.dumps(value)).decode())
126128
return
127129
except Exception as err:
128130
sys.stderr.write("Failed to get value for parameter '%s' in file %s due to: %s" % (

‎gpMgmt/test/behave/mgmt_utils/gpconfig.feature‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ Feature: gpconfig integration tests
7474
| float | checkpoint_completion_target | float | 0.4 | 0.5 | 0.5 | 0.5 | 0.33 | 0.33 | 0.7 | 0.7 | 0.7 |
7575
| basic string | application_name | string | xxxxxx | bodhi | 'bodhi' | bodhi | lucy | 'lucy' | bengie | 'bengie' | bengie |
7676
| string with spaces | application_name | string | yyyyyy | 'bod hi' | 'bod hi' | bod hi | 'bod hi' | 'bod hi' | 'bod hi' | 'bod hi' | bod hi |
77+
| 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 |
7778
| 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 |
7879
| empty string | application_name | string | zzzzzz | '' | '' | | '' | '' | '' | '' | |
7980
| quoted double quotes | application_name | string | zzzzzz | '"hi"' | '"hi"' | "hi" | '"hi"' | '"hi"' | '"hi"' | '"hi"' | "hi" |

0 commit comments

Comments
 (0)