refactor: Assign malloc return to a local variable first.

[iphydf]

See TokTok/hs-tokstyle#237.

---

This change is 

[codecov]

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (afc38f2) 69.12% compared to head (0426624) 69.00%.

Files	Patch %	Lines
toxcore/events/conference_invite.c	0.00%	4 Missing ⚠️
toxcore/events/conference_message.c	0.00%	4 Missing ⚠️
toxcore/events/conference_peer_name.c	0.00%	4 Missing ⚠️
toxcore/events/conference_title.c	0.00%	4 Missing ⚠️
toxcore/events/file_recv.c	0.00%	4 Missing ⚠️
toxcore/events/file_recv_chunk.c	0.00%	4 Missing ⚠️
toxcore/events/friend_lossless_packet.c	0.00%	4 Missing ⚠️
toxcore/events/friend_lossy_packet.c	0.00%	4 Missing ⚠️
toxcore/events/friend_request.c	0.00%	4 Missing ⚠️
toxcore/tox.c	0.00%	4 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2530      +/-   ##
==========================================
- Coverage   69.12%   69.00%   -0.13%     
==========================================
  Files          96       96              
  Lines       27983    28023      +40     
==========================================
- Hits        19343    19337       -6     
- Misses       8640     8686      +46     

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

[nurupo]

Reviewed 23 of 29 files at r1, 2 of 3 files at r2, 1 of 2 files at r3, 5 of 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @Green-Sky and @iphydf)

---

toxcore/group_announce.c line 371 at r5 (raw file):

        announces = new_announces;
    }

The original code seemed perfectly fine. announces is a local variable and assigning nullptr to it as the result of calloc() and the doing return doesn't result in any side-effects.
I understand that adding additional analysis on local variables of whether assigning nullptr and returning would cause any side-effect outside the function scope might be asking too much out of cimple, but thought I would at least point this out.
You can just ack this comment / mark it as resolved.

---

toxcore/group_chats.c line 4058 at r5 (raw file):

        memcpy(tmp, chat->shared_state.password, oldlen);
        oldpasswd = tmp;

The original oldpasswd name is better than tmp, can we do something about it?
Could you rename it to tmp_oldpasswd?

Also, the original code seemed perfectly fine. oldpasswd is a local variable and assigning nullptr to it as the result of malloc() and the doing return doesn't result in any side-effects.
I understand that adding additional analysis on local variables of whether assigning nullptr and returning would cause any side-effect outside the function scope might be asking too much out of cimple, but thought I would at least point this out.

---

toxcore/group_connection.c line 106 at r5 (raw file):

        if (entry_data == nullptr) {
            array_entry->data = nullptr;  // TODO(iphydf): Is this necessary?

@JFreegman can you chime in on this one? Is this assignment necessary? Do we also want to set array_entry->data_length = 0 along with array_entry->data = nullptr, or is it fine to keep the non-zero length?

@iphydf's change reproduces what the original code did, but I wonder if the original behavior of having array_entry->data = nullptr and non-zero array_entry->data_length is intentional, as that smells like it could result in some logic bug somewhere else. (For example, if some code somewhere else assumes that a non-zero length implies the data is non-null and thus doesn't do a check for the null.)

---

toxcore/mono_time.c line 122 at r5 (raw file):

#ifndef ESP_PLATFORM
    pthread_rwlock_t *mutex = (pthread_rwlock_t *)mem_alloc(mem, sizeof(pthread_rwlock_t));

Could you change it to lock, similar to what the original name was? No need to rename it to mutex.
(It's also incorrect to call a rwlock a mutex, as rwlock allows multiple readers or one writer, while mutex doesn't differentiate between a reader and writer and allows only one to access it at a time.)

[JFreegman]

Reviewed 1 of 29 files at r1, 2 of 3 files at r2, 1 of 2 files at r3, 3 of 5 files at r4, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @Green-Sky and @iphydf)

---

toxcore/group_chats.c line 4051 at r5 (raw file):

    if (oldlen > 0) {
        uint8_t *tmp = (uint8_t *)malloc(oldlen);

What does this do? oldpasswd is a local variable. Maybe I'm misunderstanding the point of this new tokstyle rule.

(I see nurupo beat me to it)

---

toxcore/group_connection.c line 106 at r5 (raw file):

Previously, nurupo wrote…

@JFreegman can you chime in on this one? Is this assignment necessary? Do we also want to set array_entry->data_length = 0 along with array_entry->data = nullptr, or is it fine to keep the non-zero length?

@iphydf's change reproduces what the original code did, but I wonder if the original behavior of having array_entry->data = nullptr and non-zero array_entry->data_length is intentional, as that smells like it could result in some logic bug somewhere else. (For example, if some code somewhere else assumes that a non-zero length implies the data is non-null and thus doesn't do a check for the null.)

The way we check if an array entry is empty is by checking if the time_added field is 0 via the function array_entry_is_empty(). So in other words yes it's intentional, and yes it could lead to bugs in the future if someone doesn't use the correct method of checking if the entry is empty. However, this function should only ever be called with an empty array entry, so it shouldn't be possible for the length field to be non-zero if that malloc fails. We could add a redundant empty check at the start of the function to future-proof its intended behaviour, or we could clear the array on failure via clear_array_entry(). Or we could leave it as-is and hope that future reviewers catch any bugs.

---

toxcore/group_moderation.c line 65 at r5 (raw file):

        if (entry == nullptr) {
            tmp_list[i] = nullptr;  // TODO(iphydf): Is this needed?

I don't think so since it's already null.

[iphydf]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @Green-Sky, @JFreegman, and @nurupo)

---

toxcore/group_announce.c line 371 at r5 (raw file):

Previously, nurupo wrote…

The original code seemed perfectly fine. announces is a local variable and assigning nullptr to it as the result of calloc() and the doing return doesn't result in any side-effects.
I understand that adding additional analysis on local variables of whether assigning nullptr and returning would cause any side-effect outside the function scope might be asking too much out of cimple, but thought I would at least point this out.
You can just ack this comment / mark it as resolved.

Ok, to make it more obvious, I've factored out the creation-when-null.

---

toxcore/group_chats.c line 4051 at r5 (raw file):

Previously, JFreegman wrote…

What does this do? oldpasswd is a local variable. Maybe I'm misunderstanding the point of this new tokstyle rule.

(I see nurupo beat me to it)

Ok, reverted.

---

toxcore/group_chats.c line 4058 at r5 (raw file):

Previously, nurupo wrote…

The original oldpasswd name is better than tmp, can we do something about it?
Could you rename it to tmp_oldpasswd?

Also, the original code seemed perfectly fine. oldpasswd is a local variable and assigning nullptr to it as the result of malloc() and the doing return doesn't result in any side-effects.
I understand that adding additional analysis on local variables of whether assigning nullptr and returning would cause any side-effect outside the function scope might be asking too much out of cimple, but thought I would at least point this out.

Reverted.

---

toxcore/group_connection.c line 106 at r5 (raw file):

Previously, JFreegman wrote…

The way we check if an array entry is empty is by checking if the time_added field is 0 via the function array_entry_is_empty(). So in other words yes it's intentional, and yes it could lead to bugs in the future if someone doesn't use the correct method of checking if the entry is empty. However, this function should only ever be called with an empty array entry, so it shouldn't be possible for the length field to be non-zero if that malloc fails. We could add a redundant empty check at the start of the function to future-proof its intended behaviour, or we could clear the array on failure via clear_array_entry(). Or we could leave it as-is and hope that future reviewers catch any bugs.

"hope that reviewers catch bugs" is not a strategy for secure software. Can you make a PR after this one to resolve the TODO and make things safer?

---

toxcore/group_moderation.c line 65 at r5 (raw file):

Previously, JFreegman wrote…

I don't think so since it's already null.

Done.

---

toxcore/mono_time.c line 122 at r5 (raw file):

Previously, nurupo wrote…

Could you change it to lock, similar to what the original name was? No need to rename it to mutex.
(It's also incorrect to call a rwlock a mutex, as rwlock allows multiple readers or one writer, while mutex doesn't differentiate between a reader and writer and allows only one to access it at a time.)

Done.

[JFreegman]

Reviewed 21 of 29 files at r1, 1 of 5 files at r4, 1 of 2 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @Green-Sky, @iphydf, and @nurupo)

---

toxcore/net_crypto.c line 2032 at r6 (raw file):

    }

    New_Connection n_c;

Should n_c be initialized to zero? it's not clear that the lines below won't be using the uninitialized fields.

[nurupo]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: 1 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, and @JFreegman)

---

toxcore/group_connection.c line 106 at r5 (raw file):

Previously, iphydf wrote…

"hope that reviewers catch bugs" is not a strategy for secure software. Can you make a PR after this one to resolve the TODO and make things safer?

@JFreegman I expected you to reply with something definitive and actionable like "let's do X", but instead got a hand-wavy multiple-choice "we could do X, Y, or Z" etc. so which one is it? :D You are the codeowner here, just pick whatever you think is the best and ask @iphydf to correct it!
Agree with @iphydf on having @JFreegman open a PR afterwards addressing this, this is a bit too much of communication/coordination friction and @JFreegman can do it on his own without involving @iphydf. Also this PR doesn't change the behavior, so accepting this PR without @JFreegman's changes is acceptable.

@JFreegman You say that "we check if an array entry is empty is by checking if the time_added field is 0 via the function array_entry_is_empty()", but while I see the entry being set to empty by setting data to nullptr, I don't see its time_added correspondingly being set to 0 in create_array_entry() function. Why not set it all in one place? Also, the function takes mono_time argument which is unused for some reason? Anyway, open a new PR :P

---

toxcore/group_chats.c line 4058 at r5 (raw file):

Previously, iphydf wrote…

Reverted.

Ack the revert.

(Just to make it clear, what I meant is: if possible, I would like for you to improve cimple to have it recognize that there are no issues with what the code does in this particular and similar cases and not trip on them. However if that's a too big of an ask (hard to implement, or not a priority, or you just don't want to) then this change is passable; while I'm not too happy with it because it's redundant in this case, I understand why the cimple check is done and it does make things better overall, just wish cimple was smarter about it.)

[JFreegman]

Reviewable status: 0 of 1 approvals obtained (waiting on @Green-Sky, @iphydf, and @nurupo)

---

toxcore/group_connection.c line 106 at r5 (raw file):

Previously, nurupo wrote…

@JFreegman I expected you to reply with something definitive and actionable like "let's do X", but instead got a hand-wavy multiple-choice "we could do X, Y, or Z" etc. so which one is it? :D You are the codeowner here, just pick whatever you think is the best and ask @iphydf to correct it!
Agree with @iphydf on having @JFreegman open a PR afterwards addressing this, this is a bit too much of communication/coordination friction and @JFreegman can do it on his own without involving @iphydf. Also this PR doesn't change the behavior, so accepting this PR without @JFreegman's changes is acceptable.

@JFreegman You say that "we check if an array entry is empty is by checking if the time_added field is 0 via the function array_entry_is_empty()", but while I see the entry being set to empty by setting data to nullptr, I don't see its time_added correspondingly being set to 0 in create_array_entry() function. Why not set it all in one place? Also, the function takes mono_time argument which is unused for some reason? Anyway, open a new PR :P

A null data field with a 0 length field doesn't indicate an empty entry, as there can be packets with no data that are just used as an indication of something. That's why we allow packets with a null data pointer, and use time_added instead to do the empty check. @iphydf On second thought, the assigning data to null is not actually necessary. There's no case where it isn't null already. You could call clear_array_entry() if you really want to make sure the entry gets cleared, but it's unnecessary.

As for a definitive answer, I'll think about it some more and open a pull request after this one is merged (though I'd dispute the idea that I'm the code owner). To be clear, the current code is correct as far as I can tell. It's just not future-bug-proof.

[JFreegman]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @Green-Sky, @iphydf, and @nurupo)

---

toxcore/group_connection.c line 97 at r6 (raw file):

    if (length == 0) {
        array_entry->data = nullptr;
        array_entry->data_length = 0;

These two fields are being assigned twice if this clause is triggered. The second time below is with a memcpy(ptr, nullptr, 0). Can't recall if memcpy with a src=null and length=0 is UB or not (I think it might be), but it's not exactly an improvement either way. The memcpy should only be called if there's something there to copy.

Edit: Nevermind, just misread the end bracer here.

[JFreegman]

toxcore/group_connection.c line 106 at r5 (raw file):

Previously, JFreegman wrote…

A null data field with a 0 length field doesn't indicate an empty entry, as there can be packets with no data that are just used as an indication of something. That's why we allow packets with a null data pointer, and use time_added instead to do the empty check. @iphydf On second thought, the assigning data to null is not actually necessary. There's no case where it isn't null already. You could call clear_array_entry() if you really want to make sure the entry gets cleared, but it's unnecessary.

As for a definitive answer, I'll think about it some more and open a pull request after this one is merged (though I'd dispute the idea that I'm the code owner). To be clear, the current code is correct as far as I can tell. It's just not future-bug-proof.

Actually the current code is possibly not correct, blocking until that's addressed.

[JFreegman]

Reviewed 1 of 5 files at r4, 1 of 1 files at r7, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @Green-Sky, @iphydf, and @nurupo)

[nurupo]

Reviewed all commit messages.
Reviewable status: 1 of 2 approvals obtained (waiting on @Green-Sky, @iphydf, and @JFreegman)

---

toxcore/group_connection.c line 106 at r5 (raw file):

Also, the function takes mono_time argument which is unused for some reason?

Oh, my bad, it is used, had to expand the code in Reviewable, thought the function ended earlier.

[Green-Sky]

Reviewed 1 of 1 files at r8.
Reviewable status: 1 change requests, 1 of 2 approvals obtained (waiting on @iphydf and @JFreegman)

---

toxcore/net_crypto.c line 1889 at r9 (raw file):

        c->crypto_connections[id].last_packets_left_rem = 0;
        c->crypto_connections[id].packet_send_rate_requested = 0;
        c->crypto_connections[id].last_packets_left_requested_rem = 0;

please use float/double literals, so the meaning is obvious.
(0.f and 0. or 0.0f and 0.0)

[iphydf]

Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @Green-Sky and @JFreegman)

---

toxcore/group_connection.c line 97 at r6 (raw file):

Previously, JFreegman wrote…

These two fields are being assigned twice if this clause is triggered. The second time below is with a memcpy(ptr, nullptr, 0). Can't recall if memcpy with a src=null and length=0 is UB or not (I think it might be), but it's not exactly an improvement either way. The memcpy should only be called if there's something there to copy.

No memcpy with nullptr happening anywhere.

---

toxcore/net_crypto.c line 2032 at r6 (raw file):

Previously, JFreegman wrote…

Should n_c be initialized to zero? it's not clear that the lines below won't be using the uninitialized fields.

Done.

---

toxcore/net_crypto.c line 1889 at r9 (raw file):

Previously, Green-Sky (Erik Scholz) wrote…

please use float/double literals, so the meaning is obvious.
(0.f and 0. or 0.0f and 0.0)

Done.

[Green-Sky]

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @JFreegman)