Skip to content

Commit

Permalink
[ulib][mdns] Remove double free in mdns_free_message.
Browse files Browse the repository at this point in the history 
This commit removes a free of `m->answers->name`. As reported in issue
ZX-2430, `name` isn't a pointer but a fixed length array[0]. `m->answers->name`
decays into a pointer when passed to `free`, which
points to the same place as `m->answers`, which is also freed in the
next instruction.

This bug was found by Aaron while running static analysis. ASan was not able to
detect it during tests because the code path was not triggered in unit
tests. The issue there was that the tests that added multiple answers to
a message did not call `mdns_free_messages`. I added a destructor for the
struct that holds the test data, so the message will be freed after it
goes out of scope.

Steps to compile & reproduce:

```shell
$ ./scripts/build-zircon -A -a x64
$ ./scripts/run-zircon -A -a x64 -c \
  zircon.autorun.boot=/boot/test/sys/mdns-test
```

NOTE that to repro the double free issue, you need to revert the code
change in `mdns.c`.

ZX-2430 #done

[0]: https://fuchsia.googlesource.com/zircon/+/533e8736a9d9640fccd3cb66e400b68449087149/system/ulib/mdns/include/mdns/mdns.h#129

TEST=Compiled with ASan, run unit tests as described in commit message.

Change-Id: I600a39170d3a284e6ef5250769097a7db3f6e564
  • Loading branch information
@mvanotti
mvanotti authored and CQ bot account: [email protected] committed Oct 31, 2018
1 parent 4005f08 commit c9bff26
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 15 deletions.
23 changes: 9 additions & 14 deletions system/ulib/mdns/mdns-test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const char kRrName[] = "test_rr";
//
// TODO(kjharland): Rector existing tests to use this and reduce boilerplate.
struct test_data {
bool reset() {
test_data() {
// Init message.
mdns_init_message(&message);

Expand All @@ -27,9 +27,16 @@ struct test_data {
rr.rdata = const_cast<uint8_t*>(kRdata);
rr.rdlength = sizeof(kRdata);
rr.ttl = 42;
return true;
}

test_data(const test_data&) = delete;
test_data& operator=(const test_data&) = delete;

test_data(const test_data&&) = delete;
test_data& operator=(const test_data&&) = delete;

~test_data() { mdns_free_message(&message); }

mdns_message message;
mdns_rr rr;
};
Expand Down Expand Up @@ -164,7 +171,6 @@ static bool test_mdns_add_first_answer(void) {
BEGIN_TEST;

test_data t;
t.reset();

int retval = mdns_add_answer(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -181,7 +187,6 @@ static bool test_mdns_add_nth_answer(void) {
BEGIN_TEST;

test_data t;
t.reset();

int retval = mdns_add_answer(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand Down Expand Up @@ -214,7 +219,6 @@ static bool test_mdns_add_answer_bad_rr_type(void) {
BEGIN_TEST;

test_data t;
t.reset();
t.rr.type = (uint16_t)(RR_TYPE_A + 1); // Unsupported record type.
int retval = mdns_add_answer(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -230,7 +234,6 @@ static bool test_mdns_add_answer_bad_rr_class(void) {
BEGIN_TEST;

test_data t;
t.reset();
t.rr.clazz = (uint16_t)(RR_CLASS_IN + 1); // Unsupported record class.
int retval = mdns_add_answer(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -246,7 +249,6 @@ static bool test_mdns_add_first_authority(void) {
BEGIN_TEST;

test_data t;
t.reset();

int retval = mdns_add_authority(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -263,7 +265,6 @@ static bool test_mdns_add_nth_authority(void) {
BEGIN_TEST;

test_data t;
t.reset();

int retval = mdns_add_authority(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand Down Expand Up @@ -296,7 +297,6 @@ static bool test_mdns_add_authority_bad_rr_type(void) {
BEGIN_TEST;

test_data t;
t.reset();
t.rr.type = (uint16_t)(RR_TYPE_A + 1); // Unsupported record type.
int retval = mdns_add_authority(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -312,7 +312,6 @@ static bool test_mdns_add_authority_bad_rr_class(void) {
BEGIN_TEST;

test_data t;
t.reset();
t.rr.clazz = (uint16_t)(RR_CLASS_IN + 1); // Unsupported record class.
int retval = mdns_add_authority(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -328,7 +327,6 @@ static bool test_mdns_add_first_additional(void) {
BEGIN_TEST;

test_data t;
t.reset();

int retval = mdns_add_additional(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -345,7 +343,6 @@ static bool test_mdns_add_nth_additional(void) {
BEGIN_TEST;

test_data t;
t.reset();

int retval = mdns_add_additional(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand Down Expand Up @@ -378,7 +375,6 @@ static bool test_mdns_add_additional_bad_rr_type(void) {
BEGIN_TEST;

test_data t;
t.reset();
t.rr.type = (uint16_t)(RR_TYPE_A + 1); // Unsupported record type.
int retval = mdns_add_additional(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand All @@ -394,7 +390,6 @@ static bool test_mdns_add_additional_bad_rr_class(void) {
BEGIN_TEST;

test_data t;
t.reset();
t.rr.clazz = (uint16_t)(RR_CLASS_IN + 1); // Unsupported record class.
int retval = mdns_add_additional(&t.message, t.rr.name, t.rr.type, t.rr.clazz,
t.rr.rdata, t.rr.rdlength, t.rr.ttl);
Expand Down
1 change: 0 additions & 1 deletion system/ulib/mdns/mdns.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ void mdns_free_message(mdns_message* m) {
// Free all answers in this message.
while (m->answers != NULL) {
next_rr = m->answers->next;
free(m->answers->name);
free(m->answers);
m->answers = next_rr;
}
Expand Down

0 comments on commit c9bff26

Please sign in to comment.