Skip to content
This repository was archived by the owner on Jan 20, 2025. It is now read-only.

Fix improper _recv() callback return when ssl layer returned SSL_CLOSE_NOTIFY #84

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Fix improper _recv() callback return when ssl layer returned SSL_CLOSE_NOTIFY #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ESPAsyncTCP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ err_t AsyncClient::_recv(tcp_pcb* pcb, pbuf* pb, err_t err) {
ASYNC_TCP_DEBUG("_recv err: %d\n", read_bytes);
_close();
}
return read_bytes;
//return read_bytes;
Copy link

Choose a reason for hiding this comment

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

Shouldn't we return error instead of saying ERR_OK?

Copy link
Contributor Author

@Adam5Wu Adam5Wu Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not really:

  1. if tcp_ssl_read() returns SSL_CLOSE_NOTIFY
    • The pbuf has been freed in tcp_ssl_read(), so LwIP should not try to further maintain it, and
    • Freeing pbuf in tcp_ssl_read() is a good choice because by definition of this signal, we are closing the connection anyway.
  2. if tcp_ssl_read() returns other error code
    • The block above the return is closing the connection anyway, so we don't care the left over data in pbuf
    • There are two code paths in tcp_ssl_read(), one frees the pbuf, one does not (handshake error)
      • For the code path that does not free pbuf, a reference is left in fd_data->tcp_pbuf, which will be freed when tcp_ssl_free() is called later.
    • Therefore, it is still necessary and correct to let LwIP not further maintain the pbuf.

So, in short, when we run into read_bytes < 0 situation, we will be closing the connection one way or another, and it is best to let LwIP give up ownership of the current pbuf.

And the easiest way to let LwIP do what we want is to return ERR_OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a different way to look at it, SSL belongs to "upper" layer and TCP belongs to "lower" layer.

SSL_CLOSE_NOTIFY is an upper layer signal, it may mean some abnormal situation, only to the SSL layer.

To the lower TCP layer, everything is normal -- the TCP layer successfully delivered the close notification signal to the SSL layer, hence ERR_OK.

}
return ERR_OK;
}
Expand Down
1 change: 1 addition & 0 deletions src/tcp_axtls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,7 @@ int tcp_ssl_read(struct tcp_pcb *tcp, struct pbuf *p) {
} while (p->tot_len - fd_data->pbuf_offset > 0);

tcp_recved(tcp, p->tot_len);
fd_data->tcp_pbuf = NULL;
pbuf_free(p);

return total_bytes;
Expand Down
1 change: 1 addition & 0 deletions src/tcp_axtls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
extern "C" {
#endif

#include <stdbool.h>
#include "include/ssl.h"

#define ERR_TCP_SSL_INVALID_SSL -101
Expand Down