Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a double free of the attributes block #1244

Merged
merged 1 commit into from
Aug 12, 2022
Merged

Fix a double free of the attributes block #1244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 22 additions & 32 deletions client/attribute.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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