Fix buffer overflow in parseTimeLineHistory when timeline > 1024 - #1139
Merged
Conversation
TimeLineHistory.history was a fixed-size array of 1024 entries. When a cluster's current timeline exceeded 1024, the TIMELINE_HISTORY replication command returns more lines than the array can hold. parseTimeLineHistory would then write a "tip" entry at history[1024] — one slot past the end — corrupting fields that follow ReplicationSource in LocalPostgresServer, including pgIsRunning, currentLSN, and pgKind. The keeper then logged "nodeKindToString: unknown node kind" and behaved incorrectly. Replace the fixed array with a dynamically-allocated pointer. The buffer is allocated on first use inside parseTimeLineHistory() and grown with realloc() when needed (doubling strategy), so it handles any number of timelines without a hard cap. Because ReplicationSource is embedded in the long-lived Keeper struct, the buffer is reused across calls and freed by the OS on exit — no explicit cleanup path is required. Consumer code (cli_do_misc.c, pgsql.c) uses history[index] pointer subscript syntax throughout, which is unchanged whether history is an array or a heap pointer — no consumer edits needed.
Timeline history cannot shrink (each new TLI's history file is the parent's verbatim plus one appended line, and timelines are strictly increasing), so stale entries above count are never reached by existing consumers. Defensively zero the newly allocated region after realloc so that a future consumer reading past count by mistake gets zeroes rather than garbage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a pg_auto_failover cluster has undergone more than ~1024 failovers, its current PostgreSQL timeline number exceeds 1024. At that point the
TIMELINE_HISTORYreplication command returns more than 1024 lines, overflowing the fixed-sizehistory[PG_AUTOCTL_MAX_TIMELINES]array insideTimeLineHistory.parseTimeLineHistorywrites a "tip" entry unconditionally after the loop athistory[count], wherecounthas just reached 1024 — one slot past the end of the array. Each additional timeline pushes that write further into memory. The array is the last field ofIdentifySystem, which is the last field ofReplicationSource, which is embedded inLocalPostgresServer. The overflow therefore corrupts the fields that immediately follow in that struct:pgIsRunning,pgsrSyncState,currentLSN,pgFirstStartFailureTs,pgStartRetries, andpgKind.The keeper then logs:
and behaves incorrectly because
pgKindnow holds garbage bytes from the overwritten LSN data.This bug is in the keeper (client code,
src/bin/common/pgsql.c). The monitor is not involved.Fix
Replace the fixed-size array in
TimeLineHistorywith a heap-allocated pointer:parseTimeLineHistory()allocates the buffer on first use and grows it withrealloc()(doubling strategy) when needed. BecauseReplicationSourceis embedded in the long-livedKeeperstruct, the buffer is reused across repeatedpgctl_identify_system()calls and freed by the OS on process exit — no explicit cleanup path is needed.All consumer sites (
cli_do_misc.c,pgsql.c:3041) already usehistory[index]pointer-subscript syntax, which is syntactically identical whetherhistoryis a fixed array or a heap pointer — no changes to those callers are required.Closes: #1107