Skip to content

Commit

Permalink
Fix a double free of the attributes block
Browse files Browse the repository at this point in the history 
I had no idea where the double free came from, so I just refactored to avoid
explicit delete[] altogether.

Closes #1224.
  • Loading branch information
@lmoureaux
lmoureaux committed Aug 12, 2022
1 parent 7be9ab5 commit 3ee80ef
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 112 deletions.
54 changes: 22 additions & 32 deletions client/attribute.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ void attribute_free()
Serialize an attribute hash for network/storage.
*/
static enum attribute_serial serialize_hash(attributeHash *hash,
void **pdata, int *pdata_length)
QByteArray &data)
{
/*
* Layout of version 2:
Expand All @@ -101,7 +101,6 @@ static enum attribute_serial serialize_hash(attributeHash *hash,
int total_length;
std::vector<int> value_lengths;
value_lengths.resize(entries);
void *result;
struct raw_data_out dout;
int i;

Expand Down Expand Up @@ -129,8 +128,8 @@ static enum attribute_serial serialize_hash(attributeHash *hash,
/*
* Step 2: allocate memory.
*/
result = new char[total_length];
dio_output_init(&dout, result, total_length);
data.resize(total_length);
dio_output_init(&dout, data.data(), total_length);

/*
* Step 3: fill out the preamble.
Expand Down Expand Up @@ -169,9 +168,7 @@ static enum attribute_serial serialize_hash(attributeHash *hash,
/*
* Step 5: return.
*/
*pdata = result;
*pdata_length = total_length;
log_attribute("attribute.c serialize_hash() "
log_attribute("attribute.cpp serialize_hash() "
"serialized %lu entries in %lu bytes",
(long unsigned) entries, (long unsigned) total_length);
return A_SERIAL_OK;
Expand All @@ -183,42 +180,42 @@ static enum attribute_serial serialize_hash(attributeHash *hash,
opaque data packet back to the client, and now is ready to be restored.
Check everything!
*/
static enum attribute_serial
unserialize_hash(attributeHash *hash, const void *data, size_t data_length)
static enum attribute_serial unserialize_hash(attributeHash *hash,
const QByteArray &data)
{
int entries, i, dummy;
struct data_in din;

hash->clear();

dio_input_init(&din, data, data_length);
dio_input_init(&din, data.constData(), data.size());

fc_assert_ret_val(dio_get_uint32_raw(&din, &dummy), A_SERIAL_FAIL);
if (dummy != 0) {
qDebug("attribute.c unserialize_hash() preamble, uint32 %lu != 0",
qDebug("attribute.cpp unserialize_hash() preamble, uint32 %lu != 0",
static_cast<long unsigned>(dummy));
return A_SERIAL_OLD;
}
fc_assert_ret_val(dio_get_uint8_raw(&din, &dummy), A_SERIAL_FAIL);
if (dummy != 2) {
qDebug("attribute.c unserialize_hash() preamble, "
qDebug("attribute.cpp unserialize_hash() preamble, "
"uint8 %lu != 2 version",
static_cast<long unsigned>(dummy));
return A_SERIAL_OLD;
}
fc_assert_ret_val(dio_get_uint32_raw(&din, &entries), A_SERIAL_FAIL);
fc_assert_ret_val(dio_get_uint32_raw(&din, &dummy), A_SERIAL_FAIL);
if (dummy != data_length) {
qDebug("attribute.c unserialize_hash() preamble, "
if (dummy != data.size()) {
qDebug("attribute.cpp unserialize_hash() preamble, "
"uint32 %lu != %lu data_length",
static_cast<long unsigned>(dummy),
static_cast<long unsigned>(data_length));
static_cast<long unsigned>(data.size()));
return A_SERIAL_FAIL;
}

log_attribute("attribute.c unserialize_hash() "
log_attribute("attribute.cpp unserialize_hash() "
"uint32 %lu entries, %lu data_length",
(long unsigned) entries, (long unsigned) data_length);
(long unsigned) entries, (long unsigned) data.size());

for (i = 0; i < entries; i++) {
attr_key key;
Expand All @@ -227,11 +224,11 @@ unserialize_hash(attributeHash *hash, const void *data, size_t data_length)
struct raw_data_out dout;

if (!dio_get_uint32_raw(&din, &value_length)) {
qDebug("attribute.c unserialize_hash() "
qDebug("attribute.cpp unserialize_hash() "
"uint32 value_length dio_input_too_short");
return A_SERIAL_FAIL;
}
log_attribute("attribute.c unserialize_hash() "
log_attribute("attribute.cpp unserialize_hash() "
"uint32 %lu value_length",
(long unsigned) value_length);

Expand All @@ -240,7 +237,7 @@ unserialize_hash(attributeHash *hash, const void *data, size_t data_length)
|| !dio_get_uint32_raw(&din, &key.id)
|| !dio_get_sint16_raw(&din, &key.x)
|| !dio_get_sint16_raw(&din, &key.y)) {
qDebug("attribute.c unserialize_hash() "
qDebug("attribute.cpp unserialize_hash() "
"uint32 key dio_input_too_short");
return A_SERIAL_FAIL;
}
Expand All @@ -249,7 +246,7 @@ unserialize_hash(attributeHash *hash, const void *data, size_t data_length)
dio_output_init(&dout, pvalue, value_length + 4);
dio_put_uint32_raw(&dout, value_length);
if (!dio_get_memory_raw(&din, ADD_TO_POINTER(pvalue, 4), value_length)) {
qDebug("attribute.c unserialize_hash() "
qDebug("attribute.cpp unserialize_hash() "
"memory dio_input_too_short");
return A_SERIAL_FAIL;
}
Expand All @@ -272,7 +269,7 @@ unserialize_hash(attributeHash *hash, const void *data, size_t data_length)
/* This is not an error, as old clients sent overlong serialized
* attributes pre gna bug #21295, and these will be hanging around
* in savefiles forever. */
log_attribute("attribute.c unserialize_hash() "
log_attribute("attribute.cpp unserialize_hash() "
"ignored %lu trailing octets",
(long unsigned) dio_input_remaining(&din));
}
Expand All @@ -296,14 +293,8 @@ void attribute_flush()
return;
}

if (pplayer->attribute_block.data) {
delete[] pplayer->attribute_block.data;
pplayer->attribute_block.data = nullptr;
}

serialize_hash(attribute_hash,
reinterpret_cast<void **>(&pplayer->attribute_block.data),
&pplayer->attribute_block.length);
pplayer->attribute_block.clear();
serialize_hash(attribute_hash, pplayer->attribute_block);
send_attribute_block(pplayer, &client.conn);
}

Expand All @@ -321,8 +312,7 @@ void attribute_restore()

fc_assert_ret(attribute_hash != nullptr);

switch (unserialize_hash(attribute_hash, pplayer->attribute_block.data,
pplayer->attribute_block.length)) {
switch (unserialize_hash(attribute_hash, pplayer->attribute_block)) {
case A_SERIAL_FAIL:
qCritical(_("There has been a CMA error. "
"Your citizen governor settings may be broken."));
Expand Down
44 changes: 14 additions & 30 deletions common/networking/packets.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,38 +654,23 @@ void generic_handle_player_attribute_chunk(
|| chunk->offset + chunk->chunk_length > chunk->total_length
|| (chunk->offset != 0
&& chunk->total_length
!= pplayer->attribute_block_buffer.length)) {
!= pplayer->attribute_block_buffer.size())) {
// wrong attribute data
if (pplayer->attribute_block_buffer.data) {
delete[] pplayer->attribute_block_buffer.data;
pplayer->attribute_block_buffer.data = nullptr;
}
pplayer->attribute_block_buffer.length = 0;
pplayer->attribute_block_buffer.clear();
qCritical("Received wrong attribute chunk");
return;
}
// first one in a row
if (chunk->offset == 0) {
if (pplayer->attribute_block_buffer.data) {
delete[] pplayer->attribute_block_buffer.data;
pplayer->attribute_block_buffer.data = nullptr;
}
pplayer->attribute_block_buffer.data = new char[chunk->total_length]{};
pplayer->attribute_block_buffer.length = chunk->total_length;
pplayer->attribute_block_buffer.resize(chunk->total_length);
}
memcpy(pplayer->attribute_block_buffer.data + chunk->offset, chunk->data,
memcpy(pplayer->attribute_block_buffer.data() + chunk->offset, chunk->data,
chunk->chunk_length);

if (chunk->offset + chunk->chunk_length == chunk->total_length) {
// Received full attribute block
if (pplayer->attribute_block.data != nullptr) {
delete[] pplayer->attribute_block.data;
}
pplayer->attribute_block.data = pplayer->attribute_block_buffer.data;
pplayer->attribute_block.length = pplayer->attribute_block_buffer.length;

pplayer->attribute_block_buffer.data = nullptr;
pplayer->attribute_block_buffer.length = 0;
pplayer->attribute_block = pplayer->attribute_block_buffer;
pplayer->attribute_block_buffer.clear();
}
}

Expand All @@ -696,28 +681,27 @@ void send_attribute_block(const struct player *pplayer,
struct connection *pconn)
{
struct packet_player_attribute_chunk packet;
int current_chunk, chunks, bytes_left;

if (!pplayer || !pplayer->attribute_block.data) {
if (!pplayer || pplayer->attribute_block.isEmpty()) {
return;
}

fc_assert_ret(pplayer->attribute_block.length > 0
&& pplayer->attribute_block.length < MAX_ATTRIBUTE_BLOCK);
fc_assert_ret(pplayer->attribute_block.size() < MAX_ATTRIBUTE_BLOCK);

chunks = (pplayer->attribute_block.length - 1) / ATTRIBUTE_CHUNK_SIZE + 1;
bytes_left = pplayer->attribute_block.length;
auto chunks =
(pplayer->attribute_block.size() - 1) / ATTRIBUTE_CHUNK_SIZE + 1;
auto bytes_left = pplayer->attribute_block.size();

connection_do_buffer(pconn);

for (current_chunk = 0; current_chunk < chunks; current_chunk++) {
for (int current_chunk = 0; current_chunk < chunks; current_chunk++) {
int size_of_current_chunk = MIN(bytes_left, ATTRIBUTE_CHUNK_SIZE - 1);

packet.offset = (ATTRIBUTE_CHUNK_SIZE - 1) * current_chunk;
packet.total_length = pplayer->attribute_block.length;
packet.total_length = pplayer->attribute_block.size();
packet.chunk_length = size_of_current_chunk;

memcpy(packet.data, pplayer->attribute_block.data + packet.offset,
memcpy(packet.data, pplayer->attribute_block.data() + packet.offset,
packet.chunk_length);
bytes_left -= packet.chunk_length;

Expand Down
12 changes: 2 additions & 10 deletions common/player.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -578,11 +578,6 @@ static void player_defaults(struct player *pplayer)
pplayer->wonder_build_turn[i] = -1;
}

pplayer->attribute_block.data = nullptr;
pplayer->attribute_block.length = 0;
pplayer->attribute_block_buffer.data = nullptr;
pplayer->attribute_block_buffer.length = 0;

pplayer->rgb = nullptr;

memset(pplayer->multipliers, 0, sizeof(pplayer->multipliers));
Expand Down Expand Up @@ -629,11 +624,8 @@ void player_clear(struct player *pplayer, bool full)
pplayer->savegame_ai_type_name = nullptr;

// Clears the attribute blocks.
delete[] pplayer->attribute_block.data;
pplayer->attribute_block.length = 0;

delete[] pplayer->attribute_block_buffer.data;
pplayer->attribute_block_buffer.length = 0;
pplayer->attribute_block.clear();
pplayer->attribute_block_buffer.clear();

// Clears units and cities.
unit_list_iterate(pplayer->units, punit)
Expand Down
4 changes: 2 additions & 2 deletions common/player.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,8 @@ struct player {

int wonder_build_turn[B_LAST]; /* turn numbers when wonders were built */

struct attribute_block_s attribute_block;
struct attribute_block_s attribute_block_buffer;
QByteArray attribute_block;
QByteArray attribute_block_buffer;

QBitArray *tile_known;

Expand Down
28 changes: 11 additions & 17 deletions server/savegame/savegame2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4376,24 +4376,18 @@ static void sg_load_player_attributes(struct loaddata *loading,
sg_check_ret();

// Toss any existing attribute_block (should not exist)
if (plr->attribute_block.data) {
delete[] plr->attribute_block.data;
plr->attribute_block.data = nullptr;
}
plr->attribute_block.clear();

// This is a big heap of opaque data for the client, check everything!
plr->attribute_block.length = secfile_lookup_int_default(
auto length = secfile_lookup_int_default(
loading->file, 0, "player%d.attribute_v2_block_length", plrno);

if (0 > plr->attribute_block.length) {
log_sg("player%d.attribute_v2_block_length=%d too small", plrno,
plr->attribute_block.length);
plr->attribute_block.length = 0;
} else if (MAX_ATTRIBUTE_BLOCK < plr->attribute_block.length) {
if (length < 0) {
log_sg("player%d.attribute_v2_block_length=%d too small", plrno, length);
} else if (length >= MAX_ATTRIBUTE_BLOCK) {
log_sg("player%d.attribute_v2_block_length=%d too big (max %d)", plrno,
plr->attribute_block.length, MAX_ATTRIBUTE_BLOCK);
plr->attribute_block.length = 0;
} else if (0 < plr->attribute_block.length) {
length, MAX_ATTRIBUTE_BLOCK);
} else if (length > 0) {
int part_nr, parts;
size_t actual_length;
int quoted_length;
Expand All @@ -4410,7 +4404,7 @@ static void sg_load_player_attributes(struct loaddata *loading,

quoted = new char[quoted_length + 1];
quoted[0] = '\0';
plr->attribute_block.data = new char[plr->attribute_block.length]{};
plr->attribute_block.resize(length);
for (part_nr = 0; part_nr < parts; part_nr++) {
const char *current = secfile_lookup_str(
loading->file, "player%d.attribute_v2_block_data.part%d", plrno,
Expand All @@ -4431,9 +4425,9 @@ static void sg_load_player_attributes(struct loaddata *loading,
(unsigned long) quoted_length,
(unsigned long) qstrlen(quoted));

actual_length = unquote_block(quoted, plr->attribute_block.data,
plr->attribute_block.length);
fc_assert(actual_length == plr->attribute_block.length);
actual_length = unquote_block(quoted, plr->attribute_block.data(),
plr->attribute_block.size());
fc_assert(actual_length == plr->attribute_block.size());
delete[] quoted;
}
}
Expand Down
Loading

0 comments on commit 3ee80ef

Please sign in to comment.