i2p: always check the return value of Sock::Wait() #21631

pull vasild wants to merge 1 commits into bitcoin:master from vasild:SockWait_usage_fix changing 1 files +3 −1

vasild commented at 12:19 PM on April 7, 2021: member

If Sock::Wait() fails, then cancel the Accept() method.

Not checking the return value may cause an uninitialized read a few lines below when we read the occurred variable.

Spotted by MarcoFalke, thanks!
fanquake added the label P2P on Apr 7, 2021
in src/util/sock.cpp:109 in 424246e465 outdated
```
 105 | @@ -106,6 +106,7 @@ bool Sock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occur
 106 |      return true;
 107 |  #else
 108 |      if (!IsSelectableSocket(m_socket)) {
 109 | +        errno = WSAEINVAL;
```
laanwj commented at 1:51 PM on April 7, 2021:

Isn't assigning to errno from non-libc code discouraged?

vasild commented at 2:06 PM on April 7, 2021:

I don't know, why would it be discouraged? There are already some assignments elsewhere in the code.

An alternative would be to introduce Wait()'s own error codes, all of which except one map 1:1 to errno values (as returned by poll() or select()) and one extra error code for this case where the socket is not selectable. Doesn't sound nice.

laanwj commented at 2:22 PM on April 7, 2021:

I don't know. It really seems like something the operating system / C library does, not the user code.

vasild commented at 2:45 PM on April 8, 2021:

No errno, no problem.

(removed this change at the expense of a less detailed error message)
practicalswift commented at 9:07 PM on April 7, 2021: contributor

As mentioned in #21630 (comment): a case for [[nodiscard]]? :)
i2p: cancel the Accept() method if waiting on the socket errors 1c1467f51b
vasild force-pushed on Apr 8, 2021
MarcoFalke added this to the milestone 22.0 on Apr 8, 2021
vasild commented at 2:55 PM on April 8, 2021: member

424246e46...1c1467f51: ditch a change that set errno and log a generic message if Sock::Wait() fails. @practicalswift in the SendComplete() and RecvUntilTerminator() methods and in sock_tests/wait we call Wait() without checking its return value and that's ok. Do you think it is worth adding [[nodiscard]] to Wait() and complicating the callers to check it even though it is not necessary (or typecast to void to just silence the newly added warning)?
practicalswift commented at 5:45 AM on April 9, 2021: contributor

@vasild Yes, I still think it makes sense. (void)foo() is the idiomatic way to express that one is intentionally ignoring the return value of foo(). As always: explicit is better than implicit :)
vasild commented at 7:50 AM on April 9, 2021: member

@practicalswift, I agree in general. In this case however there are 6 callers in total, 3 of them check the return value and 3 would have to be cast to void. Hmm, where is the threshold?

Also, the semantic of Wait() is such that ignoring the return value is legit.

Anyway, [[nodiscard]] is wider topic and is out of the scope of this PR. I will prepare another one to flag the relevant methods of Sock with [[nodiscard]], but I am unsure whether Wait() is one of them. What do you think, given the above?
practicalswift commented at 3:04 PM on April 9, 2021: contributor

@practicalswift, I agree in general. In this case however there are 6 callers in total, 3 of them check the return value and 3 would have to be cast to void. Hmm, where is the threshold?

Also, the semantic of Wait() is such that ignoring the return value is legit.

I see your point, and your observation may be an indication that Wait could be split up in two functions to cover the two different use cases.

In other words, one function which takes Event& occurred which is annotated [[nodiscard]], and one with doesn't take (or modify) occurred at all and thus doesn't need [[nodiscard]] :)
laanwj commented at 12:33 PM on April 12, 2021: member

Code review ACK 1c1467f51b6dda92dec974eb59026c2c5ba79ed6 Checked that the exception is caught (in the same function, even). As this fixes a problem, I think whether to use [[nodiscard]] is scope for a separate PR.
practicalswift commented at 1:07 PM on April 12, 2021: contributor

cr ACK 1c1467f51b6dda92dec974eb59026c2c5ba79ed6: patch looks correct and agree with laanwj that [[nodiscard]] can be taken in a follow-up PR :)
vasild commented at 1:53 PM on April 12, 2021: member

Sock methods flagged with [[nodiscard]] in #21659.
MarcoFalke commented at 4:15 AM on April 13, 2021: member

(removed @ from OP)
MarcoFalke commented at 4:16 AM on April 13, 2021: member

(removed @ from review comment)
MarcoFalke merged this on Apr 13, 2021
MarcoFalke closed this on Apr 13, 2021
fanquake deleted a comment on Apr 13, 2021
vasild deleted the branch on Apr 13, 2021
sidhujag referenced this in commit 02aa08ea92 on Apr 13, 2021
laanwj referenced this in commit 39d597d362 on May 19, 2021
sidhujag referenced this in commit 6ae83253d2 on May 19, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Milestone
22.0

Linked (view graph)

#21630 fuzz: split FuzzedSock interface and implementation #21659 net: flag relevant Sock methods with [[nodiscard]]