Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl: Handle that inet:getopts may return {ok, []} #7522

Conversation

IngelaAndin
Copy link
Contributor

If the standard C library call getsockopt(2) returns an error for an option - then no return value is created for that option. There is not check about the socket's state in the call chain; if the inet_drv port is still open, then the getsockopt(2) call is executed.
So it should be possible that inet:getsockopt(Port, [sndbuf]) (or recbuf), for a Port that is not dead yet but the underlying socket is closed, may return {ok,[]}.

Closes 7506

@IngelaAndin IngelaAndin added team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI labels Aug 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

CT Test Results

       2 files       66 suites   51m 0s ⏱️
   770 tests    737 ✔️   33 💤 0
3 663 runs  2 919 ✔️ 744 💤 0

Results for commit 392771c.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

If the standard C library call getsockopt(2) returns an error for an option -
then no return value is created for that option. There is not check about
the socket's state in the call chain; if the inet_drv port is still open,
then the getsockopt(2) call is executed.
So it should be possible that inet:getsockopt(Port, [sndbuf]) (or recbuf),
for a Port that is not dead yet but the underlying socket is closed,
may return {ok,[]}.

Closes erlang#7506
@IngelaAndin IngelaAndin force-pushed the ingela/ssl/getopts-empty-return/GH-7506/OTP-18697 branch from ed9bee2 to 392771c Compare August 2, 2023 06:56
Copy link
Contributor

@RaimoNiskanen RaimoNiskanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering using just one clause with Opts ++ Acc instead two with [Opt | Acc] and Acc, but the suggested change can be seen as an optimization (since it does not call '++'/2) and safer since it will crash if we should get something unforeseeable from inet:getopts/2...

@IngelaAndin
Copy link
Contributor Author

@raimo It also visualizes that inet:getopts might return an empty list which I think is not completely obvious from the documentation.

@IngelaAndin IngelaAndin added this to the OTP-26.1 milestone Aug 2, 2023
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@IngelaAndin IngelaAndin merged commit 319b9a1 into erlang:maint Aug 3, 2023
13 of 15 checks passed
@IngelaAndin IngelaAndin deleted the ingela/ssl/getopts-empty-return/GH-7506/OTP-18697 branch August 3, 2023 06:21
@RaimoNiskanen RaimoNiskanen linked an issue Aug 7, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

case clause error when TLS handshake is closed abruptly
3 participants