Fix NULL pointer dereference in log calls

[nurupo]

Dereferencing session right after making sure that it's NULL is probably not a very good idea.

---

This change is 

[nurupo]

Not really sure what is another way to get the logger object in there, so just commenting it out with a TODO.

[nurupo]

We could add an additional argument for a logger and pass av->m->log from the caller in there. Not sure if that's acceptable.

[nurupo]

Found the same issue in msi_kill().

[iphydf]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

toxav/msi.c, line 139 at r1 (raw file):

int msi_kill(MSISession *session)
{
    if (session == NULL) {

This function is called exactly once, in an expression of the form (x && msi_kill(x)), making session always non-null. If it ever does become null, we've introduced a bug somewhere else in the system. Perhaps this should be an assertion, instead.

---

toxav/rtp.c, line 115 at r1 (raw file):

int rtp_send_data(RTPSession *session, const uint8_t *data, uint16_t length)
{
    if (!session) {

Unlike the msi one, this condition does not obviously always fail. I can currently statically prove that this condition is either true or not reached. If session is null here, then the program will either have segfaulted (or otherwise caused UB) or not reached this location in another way.

Still, I think if this happens, it's a serious bug we should know about immediately. @mannol why is this a warning and not an error?

---

Comments from Reviewable

[nurupo]

I have asked if it would be acceptable to add an additional Log argument to those two functions and make the called pass the log object. Since I haven't receive any reply on that, I assume it's not acceptable.

---

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

---

toxav/rtp.c, line 115 at r1 (raw file):

Previously, iphydf wrote…

Unlike the msi one, this condition does not obviously always fail. I can currently statically prove that this condition is either true or not reached. If session is null here, then the program will either have segfaulted (or otherwise caused UB) or not reached this location in another way.

Still, I think if this happens, it's a serious bug we should know about immediately. @mannol why is this a warning and not an error?

Even if the log level was changed from `WARNING`to `ERROR`, that wouldn't matter as there would be NULL pointer deference.

---

Comments from Reviewable

[nurupo]

toxav/msi.c, line 139 at r1 (raw file):

Previously, iphydf wrote…

This function is called exactly once, in an expression of the form (x && msi_kill(x)), making session always non-null. If it ever does become null, we've introduced a bug somewhere else in the system. Perhaps this should be an assertion, instead.

Should `msi_kill()` really rely on the caller to make sure `session` is not NULL?

---

Comments from Reviewable

[iphydf]

It's acceptable.

---

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

---

toxav/msi.c, line 139 at r1 (raw file):

Previously, nurupo wrote…

Should msi_kill() really rely on the caller to make sure session is not NULL?

No, but it can make it a precondition that you can assert instead of a soft error we never notice.

---

toxav/rtp.c, line 115 at r1 (raw file):

Previously, nurupo wrote…

Even if the log level was changed from WARNINGto ERROR, that wouldn't matter as there would be NULL pointer deference.

I wasn't talking about changing the log level. I was asking why it was a warning to begin with. The reason I asked is that if it's a warning and not an error, that means we may expect this to happen and handle it gracefully. If it's an error, we may consider making it fatal.

I don't think at this point in time it's good to handle programming errors gracefully. I'm not convinced it's ever good to handle programming errors gracefully.

---

Comments from Reviewable

[nurupo]

toxav/msi.c, line 139 at r1 (raw file):

Previously, iphydf wrote…

> No, but it can make it a precondition that you can assert instead of a soft error we never notice.

Added assert.

---

Comments from Reviewable

[iphydf]

---

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable