Fix network malloc(0) bug

[Diadlo]

Thanks @nurupo for the report
From IRC:

<nurupo> Diadlo: are you here? there is an issue with the networking code
<nurupo> Diadlo: the "Unix API" one on this page. click on Details https://build.tox.chat/job/libtoxcore-toktok_analyze_scan-build/115/clangScanBuildBugs/
<nurupo> Diadlo: malloc(0) is undefined behaviour, so you should guard the malloc call with an if-statement checking for 0

---

This change is 

[sudden6]

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[nurupo]

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1226 at r1 (raw file):

INT32_MAX

Btw, why do you assume that int is at least 32-bit? It might be 16-bit, then your INT32_MAX is invalid.

You want to use INT_MAX from limits.h, which is the maximum number represented by int.

---

Comments from Reviewable

[Diadlo]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1226 at r1 (raw file):

Previously, nurupo wrote…

INT32_MAX

Btw, why do you assume that int is at least 32-bit? It might be 16-bit, then your INT32_MAX is invalid.

You want to use INT_MAX from limits.h, which is the maximum number represented by int.

Sorry. Wrong count type. Fixed.
What do you think, is it really possible to have count >= INT32_MAX? Maybe remove this check + check after loop

---

Comments from Reviewable

[nurupo]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1226 at r1 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Sorry. Wrong count type. Fixed.
What do you think, is it really possible to have count >= INT32_MAX? Maybe remove this check + check after loop

I would keep the check.

What I'm @iphydf about is what would happen when you do malloc(sizeof(IP_Port) * INT32_MAX), whether the argument to the function can overflow and result in malloc() call with a very low argument value, e.g. you do malloc(2) when you meant to malloc(INT32_MAX+2), and then try to fit INT32_MAX+2 things into those 2 slots, which results in heap buffer overflow.

---

Comments from Reviewable

[Diadlo]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1226 at r1 (raw file):

Previously, nurupo wrote…

I would keep the check.

What I'm @iphydf about is what would happen when you do malloc(sizeof(IP_Port) * INT32_MAX), whether the argument to the function can overflow and result in malloc() call with a very low argument value, e.g. you do malloc(2) when you meant to malloc(INT32_MAX+2), and then try to fit INT32_MAX+2 things into those 2 slots, which results in heap buffer overflow.

Done.

---

Comments from Reviewable

[nurupo]

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

toxcore/network.c, line 1226 at r1 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Done.

That works.

---

toxcore/network.c, line 1239 at r2 (raw file):

    }

    if (count == MAX_COUNT) {

Should be count > MAX_COUNT instead, because if count == MAX_COUNT, then count * sizeof(IP_Port) <= SIZE_MAX holds (it's <= instead of == due to integer division in C which drops the fraction part, e.g. 9/5 is 1 in C), which is still valid to pass into malloc().

---

Comments from Reviewable

[Diadlo]

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

toxcore/network.c, line 1239 at r2 (raw file):

Previously, nurupo wrote…

Should be count > MAX_COUNT instead, because if count == MAX_COUNT, then count * sizeof(IP_Port) <= SIZE_MAX holds (it's <= instead of == due to integer division in C which drops the fraction part, e.g. 9/5 is 1 in C), which is still valid to pass into malloc().

Done.

---

Comments from Reviewable

[nurupo]

@Diadlo Did you forget to push the commit?

[Diadlo]

Yes. Pushed

---

Review status: 1 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1239 at r3 (raw file):

    }

    if (count > MAX_COUNT) {

I just noticed that the loop above makes sure that count <= MAX_COUNT, so this check is not really necessary. Sorry for not noticing this from the start. You could replace it with assert assert(count <= MAX_COUNT) for clarity.

---

Comments from Reviewable

[Diadlo]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1239 at r3 (raw file):

Previously, nurupo wrote…

I just noticed that the loop above makes sure that count <= MAX_COUNT, so this check is not really necessary. Sorry for not noticing this from the start. You could replace it with assert assert(count <= MAX_COUNT) for clarity.

Done.

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 1 files at r4.
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1227 at r4 (raw file):

    int32_t count = 0;

    for (cur = infos; count <= MAX_COUNT && cur != NULL; cur = cur->ai_next) {

Either this condition should be changed back to count < MAX_COUNT or the assert below should say assert(count <= MAX_COUNT+1), since after this loop ends, count can equal to MAX_COUNT+1 now.

---

Comments from Reviewable

[nurupo]

---

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

toxcore/network.c, line 1227 at r4 (raw file):

Previously, nurupo wrote…

Either this condition should be changed back to count < MAX_COUNT or the assert below should say assert(count <= MAX_COUNT+1), since after this loop ends, count can equal to MAX_COUNT+1 now.

Ok, it's fixed now.

---

Comments from Reviewable

[nurupo]

Hm, actually, now that I think about it, count should be size_t instead of int32_t. It's possible for size_t to be 64-bit, and size_t MAX_COUNT = UINT64_MAX/sizeof(IP_Port) might still end up above int32_t's max range.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[nurupo]

Yeah, it is, in fact, the case. If size_t is 64-bit unsigned integer, then SIZE_MAX is (2^64)-1. INT32_MAX is just 2^(32-1) -1. sizeof(IP_Port) is at most 19 right now (it could change in the future, so we should assume it to be variable), and SIZE_MAX/19 > INT32_MAX, so we could overflow count on platforms which have have size_t as 64-bit unsigned integer.

---

Comments from Reviewable

[Diadlo]

Done.

---

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[nurupo]

---

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Enable the checkbox allowing pushes from collaborators.

[Diadlo]

@iphydf Done