Skip to content

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

Merged
maflcko merged 1 commit intobitcoin:masterfrom
vasild:SockWait_usage_fix
Apr 13, 2021
Merged

i2p: always check the return value of Sock::Wait()#21631
maflcko merged 1 commit intobitcoin:masterfrom
vasild:SockWait_usage_fix

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Apr 7, 2021 •
edited by maflcko
Loading

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 fanquake added the P2P label Apr 7, 2021
@vasild vasild mentioned this pull request Apr 7, 2021
Merged
laanwj
laanwj reviewed Apr 7, 2021
View reviewed changes
@practicalswift
Copy link
Contributor

practicalswift commented Apr 7, 2021

As mentioned in #21630 (comment): a case for [[nodiscard]]? :)

@vasild vasild force-pushed the SockWait_usage_fix branch from 424246e to 1c1467f Compare April 8, 2021 14:46
@maflcko maflcko added this to the 22.0 milestone Apr 8, 2021
@vasild
Copy link
Contributor Author

vasild commented Apr 8, 2021

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
Copy link
Contributor

practicalswift commented Apr 9, 2021

@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
Copy link
Contributor Author

vasild commented Apr 9, 2021

@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
Copy link
Contributor

practicalswift commented Apr 9, 2021 •
edited
Loading

@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
Copy link
Member

laanwj commented Apr 12, 2021

Code review ACK 1c1467f
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
Copy link
Contributor

practicalswift commented Apr 12, 2021 •
edited by maflcko
Loading

cr ACK 1c1467f: patch looks correct and agree with laanwj that [[nodiscard]] can be taken in a follow-up PR :)

@vasild vasild mentioned this pull request Apr 12, 2021
Merged
@vasild
Copy link
Contributor Author

vasild commented Apr 12, 2021

Sock methods flagged with [[nodiscard]] in #21659.

@maflcko
Copy link
Member

maflcko commented Apr 13, 2021

(removed @ from OP)

@maflcko
Copy link
Member

maflcko commented Apr 13, 2021

(removed @ from review comment)

@maflcko maflcko merged commit 1f50f0b into bitcoin:master Apr 13, 2021
@bitcoin bitcoin deleted a comment from lax90304 Apr 13, 2021
@vasild vasild deleted the SockWait_usage_fix branch April 13, 2021 08:39
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2021
Merge bitcoin#21631: i2p: always check the return value of Sock::Wait()
02aa08e 
1c1467f i2p: cancel the Accept() method if waiting on the socket errors (Vasil Dimov)

Pull request description:

  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](bitcoin#21630 (comment)) by MarcoFalke, thanks!

ACKs for top commit:
  laanwj:
    Code review ACK 1c1467f
  practicalswift:
    cr ACK 1c1467f: patch looks correct and agree with laanwj that `[[nodiscard]]` can be taken in a follow-up PR :)

Tree-SHA512: 57fa8a03a4e055999e23121cd9ed1566a585ece0cf68b74223d8c902804cb6890218c9356d60e0560ccacc6c8542a526356c226ebd48e7b299b4572be312d49b
laanwj added a commit to bitcoin-core/gui that referenced this pull request May 19, 2021
@laanwj
Merge bitcoin/bitcoin#21659: net: flag relevant Sock methods with [[n…
39d597d 
…odiscard]]

e286cd0 net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)

Pull request description:

  Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in bitcoin/bitcoin#21631.

ACKs for top commit:
  practicalswift:
    cr ACK e286cd0: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
  laanwj:
    Code review ACK e286cd0

Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 19, 2021
@laanwj
Merge bitcoin#21659: net: flag relevant Sock methods with [[nodiscard]]
6ae8325 
e286cd0 net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)

Pull request description:

  Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in bitcoin#21631.

ACKs for top commit:
  practicalswift:
    cr ACK e286cd0: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
  laanwj:
    Code review ACK e286cd0

Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

22.0

Development

Successfully merging this pull request may close these issues.

5 participants

@vasild @practicalswift @laanwj @maflcko @fanquake