Skip to content

Commit 5e91db1

Browse files
dimitridimitri
authored
fix: hoist intToString() temporaries to avoid dangling strValue pointers (#1133)
intToString() returns an IntString struct by value. Taking .strValue from a compound literal like intToString(x).strValue produces a pointer into a struct that is destroyed at the end of the statement. Any use of that pointer after the semicolon is undefined behaviour; when the value is stored in a paramValues[] array and read by a subsequent pgsql_execute_with_params() call, it reliably reads freed stack memory. Fix: declare a named IntString variable at function scope so that .strValue remains valid across the entire parameter-building and query-execution block. Files and instance counts: monitor.c 20 instances across 13 functions coordinator.c 10 instances across 5 functions demoapp.c 8 instances across 3 functions pgctl.c 1 instance (pg_ctl_postgres args array) pgsql.c 1 instance (send_node_active_message loop) ipaddr.c 1 instance (fetchLocalIPAddress GetAddrInfo call) Clang -Wdangling-assignment is clean after this change.
1 parent 80e9573 commit 5e91db1

6 files changed

Lines changed: 102 additions & 51 deletions

File tree

‎src/bin/pg_autoctl/coordinator.c‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,12 @@ coordinator_add_node(Coordinator *coordinator, Keeper *keeper,
159159
: keeper->config.pgSetup.citusClusterName;
160160

161161
SingleValueResultContext parseContext = { { 0 }, PGSQL_RESULT_INT, false };
162+
IntString pgportStr = intToString(keeper->config.pgSetup.pgport);
163+
IntString groupIdStr = intToString(keeper->config.groupId);
162164

163165
paramValues[0] = keeper->config.hostname;
164-
paramValues[1] = intToString(keeper->config.pgSetup.pgport).strValue;
165-
paramValues[2] = intToString(keeper->config.groupId).strValue;
166+
paramValues[1] = pgportStr.strValue;
167+
paramValues[2] = groupIdStr.strValue;
166168
paramValues[3] = citusRoleStr;
167169
paramValues[4] = clusterName;
168170

@@ -230,6 +232,8 @@ coordinator_add_inactive_node(Coordinator *coordinator, Keeper *keeper,
230232
: keeper->config.pgSetup.citusClusterName;
231233

232234
SingleValueResultContext parseContext = { { 0 }, PGSQL_RESULT_INT, false };
235+
IntString pgportStr = intToString(keeper->config.pgSetup.pgport);
236+
IntString groupIdStr = intToString(keeper->config.groupId);
233237

234238
if (!coordinator_master_activate_node_returns_record(pgsql, &returnsRecord))
235239
{
@@ -248,8 +252,8 @@ coordinator_add_inactive_node(Coordinator *coordinator, Keeper *keeper,
248252
}
249253

250254
paramValues[0] = keeper->config.hostname;
251-
paramValues[1] = intToString(keeper->config.pgSetup.pgport).strValue;
252-
paramValues[2] = intToString(keeper->config.groupId).strValue;
255+
paramValues[1] = pgportStr.strValue;
256+
paramValues[2] = groupIdStr.strValue;
253257
paramValues[3] = citusRoleStr;
254258
paramValues[4] = clusterName;
255259

@@ -307,6 +311,8 @@ coordinator_activate_node(Coordinator *coordinator, Keeper *keeper,
307311
parseContext.resultType = PGSQL_RESULT_INT;
308312
parseContext.parsedOk = false;
309313

314+
IntString pgportStr = intToString(keeper->config.pgSetup.pgport);
315+
310316
if (!coordinator_master_activate_node_returns_record(pgsql, &returnsRecord))
311317
{
312318
log_error("Failed to activate node %s:%d, see above for details",
@@ -324,7 +330,7 @@ coordinator_activate_node(Coordinator *coordinator, Keeper *keeper,
324330
}
325331

326332
paramValues[0] = keeper->config.hostname;
327-
paramValues[1] = intToString(keeper->config.pgSetup.pgport).strValue;
333+
paramValues[1] = pgportStr.strValue;
328334

329335
if (!pgsql_execute_with_params(pgsql, sql,
330336
paramCount, paramTypes, paramValues,
@@ -371,9 +377,10 @@ coordinator_remove_node(Coordinator *coordinator, Keeper *keeper)
371377
int paramCount = 2;
372378
Oid paramTypes[2] = { TEXTOID, INT4OID };
373379
const char *paramValues[2];
380+
IntString pgportStr = intToString(keeper->config.pgSetup.pgport);
374381

375382
paramValues[0] = keeper->config.hostname;
376-
paramValues[1] = intToString(keeper->config.pgSetup.pgport).strValue;
383+
paramValues[1] = pgportStr.strValue;
377384

378385
if (!pgsql_execute_with_params(pgsql, sql,
379386
paramCount, paramTypes, paramValues,
@@ -605,11 +612,16 @@ coordinator_update_node_prepare(Coordinator *coordinator, Keeper *keeper)
605612
* private data handled by the coordinator, and the coordinator is going to
606613
* provide for that information itself with the following SQL query.
607614
*/
615+
IntString groupIdStr = intToString(groupId);
616+
IntString pgportStr = intToString(keeper->config.pgSetup.pgport);
617+
608618
if (supportForForce)
609619
{
610620
const int paramCount = 5;
611621
Oid paramTypes[5] = { INT4OID, TEXTOID, INT4OID, TEXTOID, INT4OID };
612622
const char *paramValues[5];
623+
IntString lockCooldownStr = intToString(
624+
keeper->config.citus_master_update_node_lock_cooldown);
613625

614626
sformat(sql,
615627
sizeof(sql),
@@ -621,12 +633,11 @@ coordinator_update_node_prepare(Coordinator *coordinator, Keeper *keeper)
621633
" and not exists"
622634
" (select 1 from pg_prepared_xacts where gid = $4)");
623635

624-
paramValues[0] = intToString(groupId).strValue;
636+
paramValues[0] = groupIdStr.strValue;
625637
paramValues[1] = keeper->config.hostname;
626-
paramValues[2] = intToString(keeper->config.pgSetup.pgport).strValue;
638+
paramValues[2] = pgportStr.strValue;
627639
paramValues[3] = transactionName;
628-
paramValues[4] = intToString(
629-
keeper->config.citus_master_update_node_lock_cooldown).strValue;
640+
paramValues[4] = lockCooldownStr.strValue;
630641

631642
if (!pgsql_execute_with_params(pgsql, sql,
632643
paramCount, paramTypes, paramValues,
@@ -652,9 +663,9 @@ coordinator_update_node_prepare(Coordinator *coordinator, Keeper *keeper)
652663
" and not exists"
653664
" (select 1 from pg_prepared_xacts where gid = $4)");
654665

655-
paramValues[0] = intToString(groupId).strValue;
666+
paramValues[0] = groupIdStr.strValue;
656667
paramValues[1] = keeper->config.hostname;
657-
paramValues[2] = intToString(keeper->config.pgSetup.pgport).strValue;
668+
paramValues[2] = pgportStr.strValue;
658669
paramValues[3] = transactionName;
659670

660671
if (!pgsql_execute_with_params(pgsql, sql,
@@ -805,7 +816,9 @@ coordinator_upsert_poolinfo_port(Coordinator *coordinator, Keeper *keeper)
805816
sformat(proxyInfo, sizeof(proxyInfo), "host=%s port=%d",
806817
keeper->config.hostname, keeper->config.pgSetup.proxyport);
807818

808-
paramValues[0] = intToString(keeper->config.groupId).strValue;
819+
IntString groupIdStr = intToString(keeper->config.groupId);
820+
821+
paramValues[0] = groupIdStr.strValue;
809822
paramValues[1] = proxyInfo;
810823

811824
if (!pgsql_execute_with_params(pgsql, sql,

‎src/bin/pg_autoctl/demoapp.c‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -481,11 +481,15 @@ demoapp_register_client(const char *pguri,
481481

482482
const Oid paramTypes[4] = { INT4OID, INT4OID, INT4OID, INT4OID };
483483
const char *paramValues[4] = { 0 };
484+
IntString clientIdStr = intToString(clientId);
485+
IntString pidStr = intToString(getpid());
486+
IntString retrySleepStr = intToString(retrySleep);
487+
IntString retryCapStr = intToString(retryCap);
484488

485-
paramValues[0] = intToString(clientId).strValue;
486-
paramValues[1] = intToString(getpid()).strValue;
487-
paramValues[2] = intToString(retrySleep).strValue;
488-
paramValues[3] = intToString(retryCap).strValue;
489+
paramValues[0] = clientIdStr.strValue;
490+
paramValues[1] = pidStr.strValue;
491+
paramValues[2] = retrySleepStr.strValue;
492+
paramValues[3] = retryCapStr.strValue;
489493

490494
pgsql_init(&pgsql, (char *) pguri, PGSQL_CONN_APP);
491495

@@ -518,9 +522,11 @@ demoapp_update_client_failovers(const char *pguri, int clientId, int failovers)
518522

519523
const Oid paramTypes[2] = { INT4OID, INT4OID };
520524
const char *paramValues[2] = { 0 };
525+
IntString clientIdStr = intToString(clientId);
526+
IntString failoversStr = intToString(failovers);
521527

522-
paramValues[0] = intToString(clientId).strValue;
523-
paramValues[1] = intToString(failovers).strValue;
528+
paramValues[0] = clientIdStr.strValue;
529+
paramValues[1] = failoversStr.strValue;
524530

525531
pgsql_init(&pgsql, (char *) pguri, PGSQL_CONN_APP);
526532

@@ -698,11 +704,15 @@ demoapp_start_client(const char *pguri, int clientId,
698704

699705
const Oid paramTypes[5] = { INT4OID, INT4OID, INT8OID, INT8OID, BOOLOID };
700706
const char *paramValues[5] = { 0 };
701-
702-
paramValues[0] = intToString(clientId).strValue;
703-
paramValues[1] = intToString(index).strValue;
704-
paramValues[2] = intToString(pgsql.retryPolicy.attempts).strValue;
705-
paramValues[3] = intToString(INSTR_TIME_GET_MICROSEC(duration)).strValue;
707+
IntString clientIdStr = intToString(clientId);
708+
IntString indexStr = intToString(index);
709+
IntString attemptsStr = intToString(pgsql.retryPolicy.attempts);
710+
IntString durationUsStr = intToString(INSTR_TIME_GET_MICROSEC(duration));
711+
712+
paramValues[0] = clientIdStr.strValue;
713+
paramValues[1] = indexStr.strValue;
714+
paramValues[2] = attemptsStr.strValue;
715+
paramValues[3] = durationUsStr.strValue;
706716
paramValues[4] = is_in_recovery ? "true" : "false";
707717

708718
if (!pgsql_execute_with_params(&pgsql, sql, 5, paramTypes, paramValues,

‎src/bin/pg_autoctl/ipaddr.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,10 @@ fetchLocalIPAddress(char *localIpAddress, int size,
7979
hints.ai_socktype = SOCK_STREAM; /* we only want TCP sockets */
8080
hints.ai_protocol = IPPROTO_TCP; /* we only want TCP sockets */
8181

82+
IntString servicePortStr = intToString(servicePort);
83+
8284
if (!GetAddrInfo(serviceName,
83-
intToString(servicePort).strValue,
85+
servicePortStr.strValue,
8486
&hints,
8587
&lookup))
8688
{

‎src/bin/pg_autoctl/monitor.c‎

Lines changed: 47 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -846,18 +846,23 @@ monitor_register_node(Monitor *monitor, char *formation,
846846
MonitorAssignedStateParseContext parseContext =
847847
{ { 0 }, assignedState, false };
848848
const char *nodeStateString = NodeStateToString(initialState);
849+
IntString portStr = intToString(port);
850+
IntString systemIdentifierStr = intToString(system_identifier);
851+
IntString desiredNodeIdStr = intToString(desiredNodeId);
852+
IntString desiredGroupIdStr = intToString(desiredGroupId);
853+
IntString candidatePriorityStr = intToString(candidatePriority);
849854

850855
paramValues[0] = formation;
851856
paramValues[1] = host;
852-
paramValues[2] = intToString(port).strValue;
857+
paramValues[2] = portStr.strValue;
853858
paramValues[3] = dbname;
854859
paramValues[4] = name == NULL ? "" : name;
855-
paramValues[5] = intToString(system_identifier).strValue;
856-
paramValues[6] = intToString(desiredNodeId).strValue;
857-
paramValues[7] = intToString(desiredGroupId).strValue;
860+
paramValues[5] = systemIdentifierStr.strValue;
861+
paramValues[6] = desiredNodeIdStr.strValue;
862+
paramValues[7] = desiredGroupIdStr.strValue;
858863
paramValues[8] = nodeStateString;
859864
paramValues[9] = nodeKindToString(kind);
860-
paramValues[10] = intToString(candidatePriority).strValue;
865+
paramValues[10] = candidatePriorityStr.strValue;
861866
paramValues[11] = quorum ? "true" : "false";
862867
paramValues[12] =
863868
IS_EMPTY_STRING_BUFFER(citusClusterName)
@@ -945,13 +950,16 @@ monitor_node_active(Monitor *monitor,
945950
MonitorAssignedStateParseContext parseContext =
946951
{ { 0 }, assignedState, false };
947952
const char *nodeStateString = NodeStateToString(currentState);
953+
IntString nodeIdStr = intToString(nodeId);
954+
IntString groupIdStr = intToString(groupId);
955+
IntString currentTLIStr = intToString(currentTLI);
948956

949957
paramValues[0] = formation;
950-
paramValues[1] = intToString(nodeId).strValue;
951-
paramValues[2] = intToString(groupId).strValue;
958+
paramValues[1] = nodeIdStr.strValue;
959+
paramValues[2] = groupIdStr.strValue;
952960
paramValues[3] = nodeStateString;
953961
paramValues[4] = pgIsRunning ? "true" : "false";
954-
paramValues[5] = intToString(currentTLI).strValue;
962+
paramValues[5] = currentTLIStr.strValue;
955963
paramValues[6] = currentLSN;
956964
paramValues[7] = pgsrSyncState;
957965

@@ -1001,7 +1009,8 @@ monitor_set_node_candidate_priority(Monitor *monitor,
10011009
int paramCount = 3;
10021010
Oid paramTypes[3] = { TEXTOID, TEXTOID, INT4OID };
10031011
const char *paramValues[3];
1004-
char *candidatePriorityText = intToString(candidate_priority).strValue;
1012+
IntString candidatePriorityStr = intToString(candidate_priority);
1013+
char *candidatePriorityText = candidatePriorityStr.strValue;
10051014
bool success = true;
10061015

10071016
paramValues[0] = formation;
@@ -1218,8 +1227,10 @@ monitor_set_formation_number_sync_standbys(Monitor *monitor, char *formation,
12181227
Oid paramTypes[2] = { TEXTOID, INT4OID };
12191228
const char *paramValues[2];
12201229
SingleValueResultContext parseContext = { { 0 }, PGSQL_RESULT_BOOL, false };
1230+
IntString numberSyncStandbysStr = intToString(numberSyncStandbys);
1231+
12211232
paramValues[0] = formation;
1222-
paramValues[1] = intToString(numberSyncStandbys).strValue;
1233+
paramValues[1] = numberSyncStandbysStr.strValue;
12231234

12241235
if (!pgsql_execute_with_params(pgsql, sql,
12251236
paramCount, paramTypes, paramValues,
@@ -1256,9 +1267,10 @@ monitor_remove_by_hostname(Monitor *monitor, char *host, int port, bool force,
12561267
int paramCount = 3;
12571268
Oid paramTypes[3] = { TEXTOID, INT4OID, BOOLOID };
12581269
const char *paramValues[3];
1270+
IntString portStr = intToString(port);
12591271

12601272
paramValues[0] = host;
1261-
paramValues[1] = intToString(port).strValue;
1273+
paramValues[1] = portStr.strValue;
12621274
paramValues[2] = force ? "true" : "false";
12631275

12641276
if (!pgsql_execute_with_params(pgsql, sql,
@@ -1495,9 +1507,10 @@ monitor_perform_failover(Monitor *monitor, char *formation, int group)
14951507
int paramCount = 2;
14961508
Oid paramTypes[2] = { TEXTOID, INT4OID };
14971509
const char *paramValues[2];
1510+
IntString groupStr = intToString(group);
14981511

14991512
paramValues[0] = formation;
1500-
paramValues[1] = intToString(group).strValue;
1513+
paramValues[1] = groupStr.strValue;
15011514

15021515
/*
15031516
* pgautofailover.perform_failover() returns VOID.
@@ -2738,12 +2751,13 @@ monitor_create_formation(Monitor *monitor,
27382751
int paramCount = 5;
27392752
Oid paramTypes[5] = { TEXTOID, TEXTOID, TEXTOID, BOOLOID, INT4OID };
27402753
const char *paramValues[5];
2754+
IntString numberSyncStandbysStr = intToString(numberSyncStandbys);
27412755

27422756
paramValues[0] = formation;
27432757
paramValues[1] = kind;
27442758
paramValues[2] = dbname;
27452759
paramValues[3] = hasSecondary ? "true" : "false";
2746-
paramValues[4] = intToString(numberSyncStandbys).strValue;
2760+
paramValues[4] = numberSyncStandbysStr.strValue;
27472761

27482762
if (!pgsql_execute_with_params(pgsql, sql,
27492763
paramCount, paramTypes, paramValues,
@@ -3429,11 +3443,13 @@ monitor_update_node_metadata(Monitor *monitor,
34293443
const char *paramValues[4];
34303444

34313445
SingleValueResultContext context = { { 0 }, PGSQL_RESULT_BOOL, false };
3446+
IntString nodeIdStr = intToString(nodeId);
3447+
IntString portStr = intToString(port);
34323448

3433-
paramValues[0] = intToString(nodeId).strValue;
3449+
paramValues[0] = nodeIdStr.strValue;
34343450
paramValues[1] = name;
34353451
paramValues[2] = hostname;
3436-
paramValues[3] = intToString(port).strValue;
3452+
paramValues[3] = portStr.strValue;
34373453

34383454
if (!pgsql_execute_with_params(pgsql, sql,
34393455
paramCount, paramTypes, paramValues,
@@ -3477,9 +3493,11 @@ monitor_set_node_system_identifier(Monitor *monitor,
34773493

34783494
NodeAddress node = { 0 };
34793495
NodeAddressParseContext parseContext = { { 0 }, &node, false };
3496+
IntString nodeIdStr = intToString(nodeId);
3497+
IntString systemIdentifierStr = intToString(system_identifier);
34803498

3481-
paramValues[0] = intToString(nodeId).strValue;
3482-
paramValues[1] = intToString(system_identifier).strValue;
3499+
paramValues[0] = nodeIdStr.strValue;
3500+
paramValues[1] = systemIdentifierStr.strValue;
34833501

34843502
if (!pgsql_execute_with_params(pgsql, sql,
34853503
paramCount, paramTypes, paramValues,
@@ -3525,9 +3543,11 @@ monitor_set_group_system_identifier(Monitor *monitor,
35253543
const char *paramValues[2];
35263544

35273545
SingleValueResultContext context = { 0 };
3546+
IntString groupIdStr = intToString(groupId);
3547+
IntString systemIdentifierStr = intToString(system_identifier);
35283548

3529-
paramValues[0] = intToString(groupId).strValue;
3530-
paramValues[1] = intToString(system_identifier).strValue;
3549+
paramValues[0] = groupIdStr.strValue;
3550+
paramValues[1] = systemIdentifierStr.strValue;
35313551

35323552
if (!pgsql_execute_with_params(pgsql, sql,
35333553
paramCount, paramTypes, paramValues,
@@ -3640,8 +3660,9 @@ monitor_start_maintenance(Monitor *monitor, int64_t nodeId, bool *mayRetry)
36403660
int paramCount = 1;
36413661
Oid paramTypes[1] = { INT8OID };
36423662
const char *paramValues[1];
3663+
IntString nodeIdStr = intToString(nodeId);
36433664

3644-
paramValues[0] = intToString(nodeId).strValue;
3665+
paramValues[0] = nodeIdStr.strValue;
36453666

36463667
if (!pgsql_execute_with_params(pgsql, sql,
36473668
paramCount, paramTypes, paramValues,
@@ -3688,8 +3709,9 @@ monitor_stop_maintenance(Monitor *monitor, int64_t nodeId, bool *mayRetry)
36883709
int paramCount = 1;
36893710
Oid paramTypes[1] = { INT8OID };
36903711
const char *paramValues[1];
3712+
IntString nodeIdStr = intToString(nodeId);
36913713

3692-
paramValues[0] = intToString(nodeId).strValue;
3714+
paramValues[0] = nodeIdStr.strValue;
36933715

36943716
if (!pgsql_execute_with_params(pgsql, sql,
36953717
paramCount, paramTypes, paramValues,
@@ -4854,10 +4876,12 @@ monitor_find_node_by_nodeid(Monitor *monitor,
48544876
const char *paramValues[3];
48554877

48564878
NodeAddressArrayParseContext parseContext = { { 0 }, nodesArray, false };
4879+
IntString groupIdStr = intToString(groupId);
4880+
IntString nodeIdStr = intToString(nodeId);
48574881

48584882
paramValues[0] = formation;
4859-
paramValues[1] = intToString(groupId).strValue;
4860-
paramValues[2] = intToString(nodeId).strValue;
4883+
paramValues[1] = groupIdStr.strValue;
4884+
paramValues[2] = nodeIdStr.strValue;
48614885

48624886
if (!pgsql_execute_with_params(pgsql, sql,
48634887
paramCount, paramTypes, paramValues,

‎src/bin/pg_autoctl/pgctl.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1628,11 +1628,13 @@ pg_ctl_postgres(const char *pg_ctl, const char *pgdata, int pgport,
16281628
/* prepare startup.log file in PGDATA */
16291629
join_path_components(logfile, pgdata, "startup.log");
16301630

1631+
IntString pgportStr = intToString(pgport);
1632+
16311633
args[argsIndex++] = (char *) postgres;
16321634
args[argsIndex++] = "-D";
16331635
args[argsIndex++] = (char *) pgdata;
16341636
args[argsIndex++] = "-p";
1635-
args[argsIndex++] = (char *) intToString(pgport).strValue;
1637+
args[argsIndex++] = pgportStr.strValue;
16361638

16371639
if (listen)
16381640
{

0 commit comments

Comments
 (0)