Warning C4715: 'd2cs_conn_set_addr' : not all control paths return a value

Question

I'm having some issues compiling a C++ build: I'm getting two warnings from the compiler.

connection.c(813): warning C4715: 'd2cs_conn_set_addr' : not all control paths return a value

This where the code points me:

extern int d2cs_conn_set_addr(t_connection * c, unsigned int addr)
{
ASSERT(c,-1);
c->addr = addr;
}

fdwatch_iocp.c(246): warning C4700: uninitialized local variable 'ret' used

Points to this piece:

if ((rw & fdwatch_type_read) && !(rw & fdwatch_type_accept) && !(orig_state & fdwatch_type_read))

{

    memset(tmpev, 0, sizeof(WSAOVERLAPPED));

    ret = WSARecv(fdw_fd(fdw_fds + idx), &wsaBuf, 1, &dummy1, &dummy2, (LPWSAOVERLAPPED)tmpev, NULL);

    if ((ret == -1) && (err = GetLastError()) != 997)

    {

        eventlog(eventlog_level_fatal, __FUNCTION__, "cannot update iocp sock %d with read state: %d", fdw_fd(fdw_fds + idx), err);

        //printf("Error %d on WSARecv\n", err);

    }

if (fdw_rw(cfd) & fdwatch_type_read && pending_ev->events == fdwatch_type_read)
{
    if (hnd(fdw_data(cfd), fdwatch_type_read) == -2)
    {
        return;
    }
    memset(tmpev, 0, sizeof(WSAOVERLAPPED));
    WSARecv(fdw_fd(fdw_fds + idx), &wsaBuf, 1, &dummy1, &dummy2, (LPWSAOVERLAPPED)tmpev, NULL);
    if ((ret == -1) && (err = GetLastError()) != 997)
    {
        eventlog(eventlog_level_fatal, __FUNCTION__, "cannot update iocp sock %d with read state: %d", fdw_fd(fdw_fds + idx), err);
        //printf("Error %d on WSARecv\n", err);
    }

I didn't code this, I'm just trying to compile with no errors.

Answer 1

The first warning is clear: the function is declared to return an int, but doesn't return anything. If it's ever called, it will cause Undefined Behaviour (UB) of the program.

For the second warning, whether UB will happen or not depends on the relationship between these two conditions:

• (rw & fdwatch_type_read) && !(rw & fdwatch_type_accept) && !(orig_state & fdwatch_type_read)
• fdw_rw(cfd) & fdwatch_type_read && pending_ev->events == fdwatch_type_read

If the second one implies the first one, all is well and the warning can safely be ignored. If it's ever possible for the first one to be false and the second one to be true, such a case will again invoke UB (since ret is only initialised if the first one is true).

---

So much for analysis, on to conclusions.

Without understanding what the program is supposed to do, it is impossible for us to say how to fix these warnings. Technically, the most stragihtforward fix would be to return something₁ from d2cs_conn_set_addr, and to initialise ret to something₂ in the other function.

Let's look at ret first.

Since both conditions involving ret seem to treat -1 as an error value, a good value for something₂ could be -1 (basically, initialise it to an error state).

However, looking at the code a bit more shows a discrepancy between the code in the two conditions you've posted. The top one calls WSARecv and assigns its return value to ret, while the second one simply calls WSARecv and ignores the return value. Therefore, it's more likely that the proper fix would be to change the second such line from

WSARecv(fdw_fd(fdw_fds + idx), &wsaBuf, 1, &dummy1, &dummy2, (LPWSAOVERLAPPED)tmpev, NULL);

to

ret = WSARecv(fdw_fd(fdw_fds + idx), &wsaBuf, 1, &dummy1, &dummy2, (LPWSAOVERLAPPED)tmpev, NULL);

(This incidentally just shows why it's a good idea to only declare variables at the point where you need them)

And now for d2cs_conn_set_addr.

This one is trickier. If you want to find a suitable return value, you'll need to inspect the call sites (where that function is called) and see what return values they expect. It's possible they will expect a success/failure return value, perhaps non-zero vs. 0, or 0 vs. -1, or something else. Such inspection would then tell you what to return. That would probably be the success code, since the function cannot fail in any obvious ways.

Unless of course that use of ASSERT(c, -1) actually expands to

if (!c) return -1;

or similar, which would give it a failure mode (and also suggest that the correct return value could be 0 or "anything other than -1" or "anything non-negative").

So, in addition to the code which calls d2cs_conn_set_addr, also check the definition of ASSERT to hopefully better understand how to fix it.

---

To summarise: this code is plain wrong, and if errors of this degree managed to slip into it, who knows what other problems lurk in there. You should steer clear of it and not use it if that's at all possible.

If you're stuck with it, it will require very thorough inspection for potential issues.