cleanup: correct a few nullable annotations

[JFreegman]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (623e3ee) 73.11% compared to head (fbe3c19) 73.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2685      +/-   ##
==========================================
- Coverage   73.11%   73.05%   -0.06%     
==========================================
  Files         148      148              
  Lines       30486    30486              
==========================================
- Hits        22291    22273      -18     
- Misses       8195     8213      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nurupo]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @JFreegman)

---

toxcore/group_chats.c line 1812 at r1 (raw file):

    if (length > 0) {
        if (!unpack_gc_sync_announce(chat, data, length)) {

unpack_gc_sync_announce() is marked as non_null(), i.e. all pointers passed to it MUST be non-null.

---

toxcore/group_chats.c line 2275 at r1 (raw file):

        uint16_t password_length;
        net_unpack_u16(data, &password_length);

net_unpack() is marked as non_null(), i.e. all pointers passed to it MUST be non-null.

Also, if the data is NULL, net_unpack() would crash trying to access the 0x0 address.

[JFreegman]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

---

toxcore/group_chats.c line 1812 at r1 (raw file):

Previously, nurupo wrote…

unpack_gc_sync_announce() is marked as non_null(), i.e. all pointers passed to it MUST be non-null.

If length is > 0 then the pointer will always be non-null (unless there's a very serious bug in network.c)

---

toxcore/group_chats.c line 2275 at r1 (raw file):

Previously, nurupo wrote…

net_unpack() is marked as non_null(), i.e. all pointers passed to it MUST be non-null.

Also, if the data is NULL, net_unpack() would crash trying to access the 0x0 address.

Again, we do a length check right above that. There's no reason to null check after a length check. It should be noted that changing these annotations doesn't modify the way the program behaves. It's just a guideline.

[nurupo]

Does the length being non-zero guarantee that the data is not NULL? In that case these changes would make sense.

But if the length is user-provided, then no.

Reviewable status: 1 change requests, 0 of 1 approvals obtained

[nurupo]

Reviewable status: complete! 1 of 1 approvals obtained

[JFreegman]

Does the length being non-zero guarantee that the data is not NULL? In that case these changes would make sense.

But if the length is user-provided, then no.

Reviewable status: 1 change requests, 0 of 1 approvals obtained

There should never be an instance where a non-zero length is paired with a null data pointer. That would indicate a serious bug. The length is never user-provided. These notations are for telling static analyzers what to expect, and if they see something unexpected, they'll tell us about it. Since both these functions are able to handle both null and non-null data pointers, analyzers shouldn't complain if they see either case.

If there were a bug and we passed a null pointer to net_unpack() for example, then the analyzer would warn us for that specific case.

[nurupo]

These notations are for telling static analyzers what to expect, and if they see something unexpected, they'll tell us about it. Since both these functions are able to handle both null and non-null data pointers, analyzers shouldn't complain if they see either case.

Yep. And there are actually a lot more of these attributes we could use, for example marking which arguments are read-only and which are read-write, or which argument is expecting a null-terminated C-string https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

[nurupo]

Too bad we can't use those in the API as they are not really portable.