Skip to content

Commit 3eee56c

Browse files
committed
fix: harden checked payload blob cleanup
Document that cloudsync_payload_blob_checked scans successful windows twice so callers understand the I/O tradeoff of guarding monolithic payload allocation. Add cloudsync_payload_context_free() and use it from SQLite and PostgreSQL checked-blob paths so malloc-backed payload buffers are released on errors and NULL results. Refactor the PostgreSQL encode pass to reuse the existing PayloadChunksState row extraction helpers, avoiding a duplicated 9-column SPI mapping while preserving the checked window semantics.
1 parent 8331d82 commit 3eee56c

6 files changed

Lines changed: 85 additions & 89 deletions

File tree

API.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,8 @@ This helper is intended for servers that still need to support old clients on a
528528

529529
Internally, CloudSync first estimates the uncompressed payload body plus header. If that estimate exceeds `max_estimated_payload_size`, the function raises a limit-exceeded error and does not materialize the payload. Empty windows return `NULL`.
530530

531+
When the check passes, this function scans the selected change window twice: once to estimate and once to encode. This avoids unsafe monolithic allocation, but successful calls are more I/O-expensive than a direct single-pass `cloudsync_payload_encode()` over the same rows.
532+
531533
**Parameters:**
532534

533535
- `since_db_version` (INTEGER/BIGINT): Start after this source database version, except rows at the same version with `seq > since_seq` are still included.

PERFORMANCE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ Each metadata table has an **index on `db_version`**, so payload generation scal
4343

4444
The legacy `cloudsync_payload_encode()` API builds one monolithic LZ4-compressed payload before transmission. For large deltas, `cloudsync_payload_chunks()` can be used instead: it streams a sequence of payload chunks bounded by the `payload_max_chunk_size` setting (default 5 MB, minimum 256 KB). If a single encoded BLOB/TEXT value is larger than the chunk budget, the value is split into transparent v3 fragments and reassembled by `cloudsync_payload_apply()` on the receiver.
4545

46+
For legacy `/check` callers that still need one monolithic payload, `cloudsync_payload_blob_checked()` performs an internal size estimate before encoding. Successful calls scan the selected change window twice (estimate, then encode), so they trade extra I/O for avoiding unsafe monolithic payload allocation when the estimate exceeds the configured limit.
47+
4648
#### Pull: Payload Application
4749

4850
```

src/cloudsync.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3045,6 +3045,12 @@ size_t cloudsync_payload_context_size (size_t *header_size) {
30453045
return sizeof(cloudsync_payload_context);
30463046
}
30473047

3048+
void cloudsync_payload_context_free (cloudsync_payload_context *payload) {
3049+
if (!payload) return;
3050+
if (payload->buffer) cloudsync_memory_free(payload->buffer);
3051+
cloudsync_memory_free(payload);
3052+
}
3053+
30483054
uint64_t cloudsync_payload_context_nrows (cloudsync_payload_context *payload) {
30493055
return payload ? payload->nrows : 0;
30503056
}

src/cloudsync.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ int cloudsync_payload_encode_step (cloudsync_payload_context *payload, clouds
115115
int cloudsync_payload_encode_final (cloudsync_payload_context *payload, cloudsync_context *data);
116116
char *cloudsync_payload_blob (cloudsync_payload_context *payload, int64_t *blob_size, int64_t *nrows);
117117
size_t cloudsync_payload_context_size (size_t *header_size);
118+
void cloudsync_payload_context_free (cloudsync_payload_context *payload);
118119
uint64_t cloudsync_payload_context_nrows (cloudsync_payload_context *payload);
119120
size_t cloudsync_payload_context_bused (cloudsync_payload_context *payload);
120121
int cloudsync_payload_get (cloudsync_context *data, char **blob, int *blob_size, int *db_version, int64_t *new_db_version);

src/postgresql/cloudsync_postgresql.c

Lines changed: 68 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -1480,6 +1480,9 @@ PG_FUNCTION_INFO_V1(cloudsync_payload_blob_checked);
14801480
Datum cloudsync_payload_blob_checked(PG_FUNCTION_ARGS) {
14811481
cloudsync_context *data = get_cloudsync_context();
14821482
bool spi_connected = false;
1483+
Portal portal = NULL;
1484+
PayloadChunksState encode_st = {0};
1485+
cloudsync_payload_context *payload = NULL;
14831486
bytea *site_id = PG_ARGISNULL(2) ? NULL : PG_GETARG_BYTEA_PP(2);
14841487
bool exclude = PG_ARGISNULL(3) ? false : PG_GETARG_BOOL(3);
14851488

@@ -1520,100 +1523,79 @@ Datum cloudsync_payload_blob_checked(PG_FUNCTION_ARGS) {
15201523
(long long)estimated, (long long)max_estimated_size)));
15211524
}
15221525

1523-
StringInfoData q;
1524-
initStringInfo(&q);
1525-
if (exclude) {
1526-
appendStringInfoString(&q,
1527-
"SELECT tbl, pk, col_name, col_value, col_version, db_version, site_id, cl, seq "
1528-
"FROM cloudsync_changes_select(0,NULL) "
1529-
"WHERE site_id <> $2 AND db_version <= $3 AND (db_version > $1 OR (db_version = $1 AND seq > $4)) "
1530-
"ORDER BY db_version, seq ASC");
1531-
} else {
1532-
appendStringInfoString(&q,
1533-
"SELECT tbl, pk, col_name, col_value, col_version, db_version, site_id, cl, seq "
1534-
"FROM cloudsync_changes_select(0,$2) "
1535-
"WHERE db_version <= $3 AND (db_version > $1 OR (db_version = $1 AND seq > $4)) "
1536-
"ORDER BY db_version, seq ASC");
1537-
}
1538-
Oid argtypes[4] = {INT8OID, BYTEAOID, INT8OID, INT8OID};
1539-
Datum values[4] = {Int64GetDatum(since), PointerGetDatum(site_id), Int64GetDatum(watermark), Int64GetDatum(since_seq)};
1540-
char nulls[4] = {' ', ' ', ' ', ' '};
1541-
Portal portal = SPI_cursor_open_with_args(NULL, q.data, 4, argtypes, values, nulls, true, 0);
1542-
pfree(q.data);
1543-
if (!portal) ereport(ERROR, (errmsg("SPI_cursor_open failed")));
1526+
PG_TRY();
1527+
{
1528+
StringInfoData q;
1529+
initStringInfo(&q);
1530+
if (exclude) {
1531+
appendStringInfoString(&q,
1532+
"SELECT tbl, pk, col_name, col_value, col_version, db_version, site_id, cl, seq "
1533+
"FROM cloudsync_changes_select(0,NULL) "
1534+
"WHERE site_id <> $2 AND db_version <= $3 AND (db_version > $1 OR (db_version = $1 AND seq > $4)) "
1535+
"ORDER BY db_version, seq ASC");
1536+
} else {
1537+
appendStringInfoString(&q,
1538+
"SELECT tbl, pk, col_name, col_value, col_version, db_version, site_id, cl, seq "
1539+
"FROM cloudsync_changes_select(0,$2) "
1540+
"WHERE db_version <= $3 AND (db_version > $1 OR (db_version = $1 AND seq > $4)) "
1541+
"ORDER BY db_version, seq ASC");
1542+
}
1543+
Oid argtypes[4] = {INT8OID, BYTEAOID, INT8OID, INT8OID};
1544+
Datum values[4] = {Int64GetDatum(since), PointerGetDatum(site_id), Int64GetDatum(watermark), Int64GetDatum(since_seq)};
1545+
char nulls[4] = {' ', ' ', ' ', ' '};
1546+
portal = SPI_cursor_open_with_args(NULL, q.data, 4, argtypes, values, nulls, true, 0);
1547+
pfree(q.data);
1548+
if (!portal) ereport(ERROR, (errmsg("SPI_cursor_open failed")));
1549+
encode_st.portal = portal;
15441550

1545-
cloudsync_payload_context *payload = cloudsync_memory_zeroalloc((uint64_t)cloudsync_payload_context_size(NULL));
1546-
if (!payload) ereport(ERROR, (errcode(ERRCODE_OUT_OF_MEMORY), errmsg("out of memory")));
1551+
payload = cloudsync_memory_zeroalloc((uint64_t)cloudsync_payload_context_size(NULL));
1552+
if (!payload) ereport(ERROR, (errcode(ERRCODE_OUT_OF_MEMORY), errmsg("out of memory")));
15471553

1548-
for (;;) {
1549-
SPI_cursor_fetch(portal, true, 1);
1550-
if (SPI_processed == 0) {
1551-
if (SPI_tuptable) { SPI_freetuptable(SPI_tuptable); SPI_tuptable = NULL; }
1552-
break;
1554+
while (payload_chunks_fetch_current(&encode_st)) {
1555+
pgvalue_t *vals[9] = {0};
1556+
text *owned_texts[2] = {0};
1557+
payload_chunks_make_pgvalues(&encode_st, vals, owned_texts);
1558+
int rc = cloudsync_payload_encode_step(payload, data, 9, (dbvalue_t **)vals);
1559+
payload_chunks_free_pgvalues(vals, owned_texts);
1560+
if (rc != DBRES_OK) {
1561+
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("%s", cloudsync_errmsg(data))));
1562+
}
1563+
payload_chunks_free_current(&encode_st);
15531564
}
1554-
SPITupleTable *tuptable = SPI_tuptable;
1555-
HeapTuple tup = tuptable->vals[0];
1556-
TupleDesc td = tuptable->tupdesc;
1557-
bool isnull = false;
1558-
Datum d;
1559-
1560-
char *tbl = NULL;
1561-
bytea *pk = NULL;
1562-
char *col_name = NULL;
1563-
bytea *col_value = NULL;
1564-
bytea *row_site_id = NULL;
1565-
int64 col_version = 0, db_version = 0, cl = 0, seq = 0;
1566-
1567-
d = SPI_getbinval(tup, td, 1, &isnull); tbl = isnull ? pstrdup("") : text_to_cstring(DatumGetTextPP(d));
1568-
d = SPI_getbinval(tup, td, 2, &isnull); if (!isnull) pk = DatumGetByteaPP(d);
1569-
d = SPI_getbinval(tup, td, 3, &isnull); col_name = isnull ? pstrdup("") : text_to_cstring(DatumGetTextPP(d));
1570-
d = SPI_getbinval(tup, td, 4, &isnull); if (!isnull) col_value = DatumGetByteaPP(d);
1571-
d = SPI_getbinval(tup, td, 5, &isnull); col_version = isnull ? 0 : DatumGetInt64(d);
1572-
d = SPI_getbinval(tup, td, 6, &isnull); db_version = isnull ? 0 : DatumGetInt64(d);
1573-
d = SPI_getbinval(tup, td, 7, &isnull); if (!isnull) row_site_id = DatumGetByteaPP(d);
1574-
d = SPI_getbinval(tup, td, 8, &isnull); cl = isnull ? 0 : DatumGetInt64(d);
1575-
d = SPI_getbinval(tup, td, 9, &isnull); seq = isnull ? 0 : DatumGetInt64(d);
1565+
SPI_cursor_close(portal);
1566+
portal = NULL;
1567+
encode_st.portal = NULL;
15761568

1577-
pgvalue_t *vals[9] = {0};
1578-
text *owned_texts[2] = {0};
1579-
owned_texts[0] = cstring_to_text(tbl);
1580-
owned_texts[1] = cstring_to_text(col_name);
1581-
vals[0] = pgvalue_create(PointerGetDatum(owned_texts[0]), TEXTOID, -1, InvalidOid, false);
1582-
vals[1] = pgvalue_create(PointerGetDatum(pk), BYTEAOID, -1, InvalidOid, false);
1583-
vals[2] = pgvalue_create(PointerGetDatum(owned_texts[1]), TEXTOID, -1, InvalidOid, false);
1584-
vals[3] = pgvalue_create(PointerGetDatum(col_value), BYTEAOID, -1, InvalidOid, false);
1585-
vals[4] = pgvalue_create(Int64GetDatum(col_version), INT8OID, -1, InvalidOid, false);
1586-
vals[5] = pgvalue_create(Int64GetDatum(db_version), INT8OID, -1, InvalidOid, false);
1587-
vals[6] = pgvalue_create(PointerGetDatum(row_site_id), BYTEAOID, -1, InvalidOid, false);
1588-
vals[7] = pgvalue_create(Int64GetDatum(cl), INT8OID, -1, InvalidOid, false);
1589-
vals[8] = pgvalue_create(Int64GetDatum(seq), INT8OID, -1, InvalidOid, false);
1590-
int rc = cloudsync_payload_encode_step(payload, data, 9, (dbvalue_t **)vals);
1591-
payload_chunks_free_pgvalues(vals, owned_texts);
1592-
pfree(tbl);
1593-
pfree(col_name);
1569+
int rc = cloudsync_payload_encode_final(payload, data);
15941570
if (rc != DBRES_OK) ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("%s", cloudsync_errmsg(data))));
1571+
int64 blob_size = 0;
1572+
char *blob = cloudsync_payload_blob(payload, &blob_size, NULL);
1573+
if (spi_connected) {
1574+
SPI_finish();
1575+
spi_connected = false;
1576+
}
1577+
if (!blob) {
1578+
cloudsync_payload_context_free(payload);
1579+
payload = NULL;
1580+
PG_RETURN_NULL();
1581+
}
15951582

1596-
if (tuptable) SPI_freetuptable(tuptable);
1597-
SPI_tuptable = NULL;
1583+
bytea *result = (bytea *)palloc(VARHDRSZ + blob_size);
1584+
SET_VARSIZE(result, VARHDRSZ + blob_size);
1585+
memcpy(VARDATA(result), blob, blob_size);
1586+
cloudsync_payload_context_free(payload);
1587+
payload = NULL;
1588+
PG_RETURN_BYTEA_P(result);
15981589
}
1599-
SPI_cursor_close(portal);
1600-
1601-
int rc = cloudsync_payload_encode_final(payload, data);
1602-
if (rc != DBRES_OK) ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("%s", cloudsync_errmsg(data))));
1603-
int64 blob_size = 0;
1604-
char *blob = cloudsync_payload_blob(payload, &blob_size, NULL);
1605-
if (spi_connected) SPI_finish();
1606-
if (!blob) {
1607-
cloudsync_memory_free(payload);
1608-
PG_RETURN_NULL();
1590+
PG_CATCH();
1591+
{
1592+
if (portal) SPI_cursor_close(portal);
1593+
payload_chunks_free_current(&encode_st);
1594+
if (payload) cloudsync_payload_context_free(payload);
1595+
if (spi_connected) SPI_finish();
1596+
PG_RE_THROW();
16091597
}
1610-
1611-
bytea *result = (bytea *)palloc(VARHDRSZ + blob_size);
1612-
SET_VARSIZE(result, VARHDRSZ + blob_size);
1613-
memcpy(VARDATA(result), blob, blob_size);
1614-
cloudsync_memory_free(blob);
1615-
cloudsync_memory_free(payload);
1616-
PG_RETURN_BYTEA_P(result);
1598+
PG_END_TRY();
16171599
}
16181600

16191601
// Payload decode - Apply changes from payload

src/sqlite/cloudsync_sqlite.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1508,6 +1508,7 @@ static int payload_blob_checked_estimate(sqlite3 *db, const void *site_id, int s
15081508
int rc = SQLITE_OK;
15091509
sqlite3_int64 until = 0;
15101510
size_t header_size = 0;
1511+
bool has_rows = false;
15111512
const char *site_op = exclude ? "<>" : "=";
15121513
*estimated = 0;
15131514

@@ -1551,13 +1552,14 @@ static int payload_blob_checked_estimate(sqlite3 *db, const void *site_id, int s
15511552
rc = SQLITE_TOOBIG;
15521553
goto error;
15531554
}
1554-
if (*estimated == 0) {
1555+
if (!has_rows) {
15551556
if (header_size > (size_t)INT64_MAX) {
15561557
rc = SQLITE_TOOBIG;
15571558
goto error;
15581559
}
15591560
rc = payload_estimated_size_add(estimated, (sqlite3_int64)header_size);
15601561
if (rc != SQLITE_OK) goto error;
1562+
has_rows = true;
15611563
}
15621564
rc = payload_estimated_size_add(estimated, (sqlite3_int64)row_size);
15631565
if (rc != SQLITE_OK) goto error;
@@ -1661,15 +1663,16 @@ void dbsync_payload_blob_checked(sqlite3_context *context, int argc, sqlite3_val
16611663
char *blob = cloudsync_payload_blob(payload, &blob_size, NULL);
16621664
if (!blob) {
16631665
sqlite3_result_null(context);
1666+
cloudsync_payload_context_free(payload);
16641667
} else {
16651668
sqlite3_result_blob64(context, blob, (sqlite3_uint64)blob_size, cloudsync_memory_free);
1669+
cloudsync_memory_free(payload);
16661670
}
1667-
cloudsync_memory_free(payload);
16681671
return;
16691672

16701673
error:
16711674
if (stmt) sqlite3_finalize(stmt);
1672-
if (payload) cloudsync_memory_free(payload);
1675+
if (payload) cloudsync_payload_context_free(payload);
16731676
if (rc == SQLITE_NOMEM) sqlite3_result_error_nomem(context);
16741677
else if (rc == SQLITE_TOOBIG) sqlite3_result_error(context, "cloudsync_payload_blob_checked: payload estimate is too large", -1);
16751678
else sqlite3_result_error(context, sqlite3_errmsg(db), -1);

0 commit comments

Comments
 (0)