You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/28 08:18:03 UTC
[GitHub] [incubator-nuttx-apps] yamt opened a new pull request #695: webclient: Implement non-blocking I/O
yamt opened a new pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695
## Summary
* webclient_perform
* Add a new flag to use non-blocking mode (WEBCLIENT_FLAG_NON_BLOCKING)
* Implement restarting
* Add a few associated API functions
* webclient_get_poll_info: get the descriptor info for poll/select
* webclient_abort: abort the operation (instead of restarting)
## Impact
## Testing
tested with my local app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839761694
> > > if the code is bug-free, the value doesn't matter.
> > > they should not be used by anyone.
> > > thus those NULLify should not be necessary.
> >
> >
> > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> > And if this single commit naively operates on this dangling pointer, the system will crash.
>
> why doesn't a local variable matter?
> with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases that the value does matter.
If a local variable goes out-of-scope (i.e. it's lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
> > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> >
> >
> > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > Now, here it is a simple unit test for making this potential bug evident:
> > https://godbolt.org/z/f4dzW8M44
>
> the unit test is just broken.
> it doesn't seem to support your argument.
Oh, really? Guess why it is broken?
```
Program stderr
free(): double free detected in tcache 2
```
I rest my case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-835642332
> the "defensive" style can make it difficult to find such bugs.
@yamt It doesn't make sense. If you are following the recommended mitigation methods and therefore preventing these bugs from happening, which bugs are you expecting to find then?
> i certainly don't recommend to apply the method blindly.
> (i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.)
Indeed, one of the issues I've raised is actually wrong and I acknowledge my mistake. However, my review process is far from being considered "blind", and that was an unfortunate statement of yours.
By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r622258955
##########
File path: include/netutils/webclient.h
##########
@@ -235,6 +240,29 @@ struct webclient_context
FAR void *body_callback_arg;
FAR const struct webclient_tls_ops *tls_ops;
FAR void *tls_ctx;
+ unsigned int flags;
+
+ /* WEBCLIENT_FLAG_NON_BLOCKING tells webclient_perform() to
+ * use non-blocking I/O.
+ *
+ * If this flag is set, webclient_perform() returns -EAGAIN
+ * when it would otherwise block for network I/O. In that case,
+ * the application should either retry the operation later by calling
+ * webclient_perform() again, or abort it by calling webclient_abort().
+ * It can also use webclient_get_poll_info() to avoid busy-retrying.
+ *
+ * If this flag is set, it's the application's responsibility to
+ * implement a timeout.
+ *
+ * If the application specifies tls_ops, it's the application's
+ * responsibility to make the TLS implementation to use non-blocking I/O
+ * in addition to specifying this flag.
+ *
+ * Caveat: Even when this flag is set, the current implementation performs
+ * the name resolution in a blocking manner.
+ */
+
+#define WEBCLIENT_FLAG_NON_BLOCKING 1U
Review comment:
Placement of this preprocessor definition is not compliant to the coding standard.
##########
File path: include/netutils/webclient.h
##########
@@ -249,6 +277,19 @@ struct webclient_context
unsigned int http_status;
FAR char *http_reason;
size_t http_reason_len;
+
+ struct wget_s *ws;
+};
+
+struct webclient_poll_info
+{
+ /* A file descriptor to wait for i/o. */
+
+ int fd;
+ unsigned int flags; /* OR'ed WEBCLIENT_POLL_INFO_xxx flags */
+
+#define WEBCLIENT_POLL_INFO_WANT_READ 1U
+#define WEBCLIENT_POLL_INFO_WANT_WRITE 2U
Review comment:
Placement of these preprocessor definition is not compliant to the coding standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r623493390
##########
File path: include/netutils/webclient.h
##########
@@ -235,6 +240,29 @@ struct webclient_context
FAR void *body_callback_arg;
FAR const struct webclient_tls_ops *tls_ops;
FAR void *tls_ctx;
+ unsigned int flags;
+
+ /* WEBCLIENT_FLAG_NON_BLOCKING tells webclient_perform() to
+ * use non-blocking I/O.
+ *
+ * If this flag is set, webclient_perform() returns -EAGAIN
+ * when it would otherwise block for network I/O. In that case,
+ * the application should either retry the operation later by calling
+ * webclient_perform() again, or abort it by calling webclient_abort().
+ * It can also use webclient_get_poll_info() to avoid busy-retrying.
+ *
+ * If this flag is set, it's the application's responsibility to
+ * implement a timeout.
+ *
+ * If the application specifies tls_ops, it's the application's
+ * responsibility to make the TLS implementation to use non-blocking I/O
+ * in addition to specifying this flag.
+ *
+ * Caveat: Even when this flag is set, the current implementation performs
+ * the name resolution in a blocking manner.
+ */
+
+#define WEBCLIENT_FLAG_NON_BLOCKING 1U
Review comment:
fixed
##########
File path: include/netutils/webclient.h
##########
@@ -249,6 +277,19 @@ struct webclient_context
unsigned int http_status;
FAR char *http_reason;
size_t http_reason_len;
+
+ struct wget_s *ws;
+};
+
+struct webclient_poll_info
+{
+ /* A file descriptor to wait for i/o. */
+
+ int fd;
+ unsigned int flags; /* OR'ed WEBCLIENT_POLL_INFO_xxx flags */
+
+#define WEBCLIENT_POLL_INFO_WANT_READ 1U
+#define WEBCLIENT_POLL_INFO_WANT_WRITE 2U
Review comment:
fixed
##########
File path: netutils/webclient/webclient.c
##########
@@ -132,6 +129,37 @@
* Private Types
****************************************************************************/
+enum webclient_state
Review comment:
fixed
##########
File path: netutils/webclient/webclient.c
##########
@@ -132,6 +129,37 @@
* Private Types
****************************************************************************/
+enum webclient_state
+ {
+ WEBCLIENT_STATE_SOCKET,
+ WEBCLIENT_STATE_CONNECT,
+ WEBCLIENT_STATE_PREPARE_REQUEST,
+ WEBCLIENT_STATE_SEND_REQUEST,
+ WEBCLIENT_STATE_SEND_REQUEST_BODY,
+ WEBCLIENT_STATE_STATUSLINE,
+ WEBCLIENT_STATE_HEADERS,
+ WEBCLIENT_STATE_DATA,
+ WEBCLIENT_STATE_CLOSE,
+ WEBCLIENT_STATE_DONE,
+ };
+
+struct conn
+{
+ bool tls;
+
+ /* for !tls */
+
+ int sockfd;
+ unsigned int flags;
+
+#define CONN_WANT_READ WEBCLIENT_POLL_INFO_WANT_READ
+#define CONN_WANT_WRITE WEBCLIENT_POLL_INFO_WANT_WRITE
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r623456837
##########
File path: include/netutils/webclient.h
##########
@@ -165,6 +165,7 @@ typedef CODE int (*webclient_body_callback_t)(
FAR void *ctx);
struct webclient_tls_connection;
+struct webclient_poll_info;
Review comment:
ok. but i will not mix style fixes into this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r622255657
##########
File path: include/netutils/webclient.h
##########
@@ -165,6 +165,7 @@ typedef CODE int (*webclient_body_callback_t)(
FAR void *ctx);
struct webclient_tls_connection;
+struct webclient_poll_info;
Review comment:
```suggestion
struct webclient_tls_connection_s;
struct webclient_poll_info_s;
```
According to the coding style, structure names must end with the suffix `_s`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-833209957
the last push was a rebase with conflict resolutions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-842853523
> This PR introduces some **use-after-free** and **double-free** potential bugs that should be addressed before merging.
i don't agree and explained why not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r622260315
##########
File path: netutils/webclient/webclient.c
##########
@@ -132,6 +129,37 @@
* Private Types
****************************************************************************/
+enum webclient_state
+ {
+ WEBCLIENT_STATE_SOCKET,
+ WEBCLIENT_STATE_CONNECT,
+ WEBCLIENT_STATE_PREPARE_REQUEST,
+ WEBCLIENT_STATE_SEND_REQUEST,
+ WEBCLIENT_STATE_SEND_REQUEST_BODY,
+ WEBCLIENT_STATE_STATUSLINE,
+ WEBCLIENT_STATE_HEADERS,
+ WEBCLIENT_STATE_DATA,
+ WEBCLIENT_STATE_CLOSE,
+ WEBCLIENT_STATE_DONE,
+ };
+
+struct conn
Review comment:
```suggestion
struct conn_s
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839761694
> > > > By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them.
> > >
> > >
> > > if the code is bug-free, the value doesn't matter.
> > > they should not be used by anyone.
> > > thus those NULLify should not be necessary.
> >
> >
> > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> > And if this single commit naively operates on this dangling pointer, the system will crash.
>
> why doesn't a local variable matter?
> with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases that the value does matter.
If a local variable goes out-of-scope (i.e. it's lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
> > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> >
> >
> > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > Now, here it is a simple unit test for making this potential bug evident:
> > https://godbolt.org/z/f4dzW8M44
>
> the unit test is just broken.
> it doesn't seem to support your argument.
Oh, really? Guess why it is broken?
```
Program stderr
free(): double free detected in tcache 2
```
I rest my case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r622260185
##########
File path: netutils/webclient/webclient.c
##########
@@ -132,6 +129,37 @@
* Private Types
****************************************************************************/
+enum webclient_state
Review comment:
```suggestion
enum webclient_state_e
```
According to the coding style, enumeration names must end with the suffix `_e`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-837530925
> > the "defensive" style can make it difficult to find such bugs.
>
> @yamt It doesn't make sense. If you are following the recommended mitigation methods and therefore preventing these bugs from happening, which bugs are you expecting to find then?
unintended uses of values.
>
> > i certainly don't recommend to apply the method blindly.
> > (i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.)
>
> Indeed, one of the issues I've raised is actually wrong and I acknowledge my mistake. However, my review process is far from being considered "blind", and that was an unfortunate statement of yours.
sorry if you felt that way. i have no intention to attack you.
"blind" might have been an inappropriate word. maybe "mechanical"?
> By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them.
if the code is bug-free, the value doesn't matter.
they should not be used by anyone.
thus those NULLify should not be necessary.
if there is a bug (i guess there is :-) i prefer it fails loudly.
that's why i generally don't like the "defensive" methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-841904553
the last push was a conflict resolution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-873766880
@gustavonihei is the nullification thing the only thing you are unhappy with this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-840884007
> > it's something like having a unit test like the following and claiming that free() has a problem.
> > it doesn't make much sense.
> > ```
> > p = malloc(1);
> > free(p);
> > free(p);
> > ```
>
> It's completely different.
> If your comparison is based on the fact that I've called the `webclient_abort` twice, feel free to create another test that calls `webclient_perform` after a single `webclient_abort`.
it's same.
webclient_perform after webclient_abort is also same.
they are all wrong use of apis.
you can invent more and more "problematic" sequences. eg. realloc after free.
but it doesn't mean these apis has problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-892171687
@xiaoxiang781216 Could you please review this PR?
Also, please evaluate whether the issue I've raised is indeed valid. I am not comfortable in merging this PR due to that issue, so a third opinion might be helpful here.
This PR has been in limbo too long, let's try to move it forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-874099283
> @gustavonihei is the nullification thing the only thing you are unhappy with this PR?
Yes.
Regarding the styling issues, I am fine with addressing them in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 merged pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r632185855
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
Review comment:
@mlyszczek
my understanding is that his concern is about ctx->ws, not ws.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628554418
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
return OK;
-errout:
- ret = -errno;
errout_with_errno:
- conn_close(ctx, &conn);
+ if ((ctx->flags & WEBCLIENT_FLAG_NON_BLOCKING) != 0 &&
+ (ret == -EAGAIN || ret == -EINPROGRESS || ret == -EALREADY))
+ {
+ if (ret == -EINPROGRESS || ret == -EALREADY)
+ {
+ conn->flags |= CONN_WANT_WRITE;
+ }
+
+ return -EAGAIN;
+ }
+
+ if (ws->need_conn_close)
+ {
+ conn_close(ctx, conn);
+ }
free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-894001196
i marked this ready again as i tested the latest version with my local app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-842850964
the last push fixed doc and debug code a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628553570
##########
File path: netutils/webclient/webclient.c
##########
@@ -1044,6 +1227,38 @@ int webclient_perform(FAR struct webclient_context *ctx)
* Public Functions
****************************************************************************/
+/****************************************************************************
+ * Name: webclient_abort
+ *
+ * Description:
+ * This function is used to free the resources used by webclient_perform()
+ * in case of non blocking I/O.
+ *
+ * After webclient_perform() returned -EAGAIN, the application can either
+ * retry the operation by calling webclient_perform() again or abort
+ * the operation by calling this function.
+ *
+ ****************************************************************************/
+
+void webclient_abort(FAR struct webclient_context *ctx)
+{
+ struct wget_s *ws = ctx->ws;
+
+ if (ws == NULL)
+ {
+ return;
+ }
+
+ if (ws->need_conn_close)
+ {
+ struct conn *conn = &ws->conn;
+
+ conn_close(ctx, conn);
+ }
+
+ free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] acassis commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628630487
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
return OK;
-errout:
- ret = -errno;
errout_with_errno:
- conn_close(ctx, &conn);
+ if ((ctx->flags & WEBCLIENT_FLAG_NON_BLOCKING) != 0 &&
+ (ret == -EAGAIN || ret == -EINPROGRESS || ret == -EALREADY))
+ {
+ if (ret == -EINPROGRESS || ret == -EALREADY)
+ {
+ conn->flags |= CONN_WANT_WRITE;
+ }
+
+ return -EAGAIN;
+ }
+
+ if (ws->need_conn_close)
+ {
+ conn_close(ctx, conn);
+ }
free(ws);
Review comment:
@yamt https://en.wikipedia.org/wiki/Dangling_pointer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-837545176
@gustavonihei
i pushed a counter-proposal to the defensive style. https://github.com/apache/incubator-nuttx-apps/pull/695/commits/219356ce7d1b8a26140c98d303d493c6f04b19e9
(it has not been tested and thus i marked this PR a draft)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839761694
> > > if the code is bug-free, the value doesn't matter.
> > > they should not be used by anyone.
> > > thus those NULLify should not be necessary.
> >
> >
> > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> > And if this single commit naively operates on this dangling pointer, the system will crash.
>
> why doesn't a local variable matter?
> with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases where the value does matter.
If a local variable goes out-of-scope (i.e. it's lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
> > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> >
> >
> > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > Now, here it is a simple unit test for making this potential bug evident:
> > https://godbolt.org/z/f4dzW8M44
>
> the unit test is just broken.
> it doesn't seem to support your argument.
Guess why it is broken?
```
Program stderr
free(): double free detected in tcache 2
```
I rest my case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839064451
> > > the "defensive" style can make it difficult to find such bugs.
> >
> >
> > @yamt It doesn't make sense. If you are following the recommended mitigation methods and therefore preventing these bugs from happening, which bugs are you expecting to find then?
>
> unintended uses of values.
Fine. So as long the recommended mitigation methods are being followed, you won't find them. Better safe than sorry.
>
> > > i certainly don't recommend to apply the method blindly.
> > > (i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.)
> >
> >
> > Indeed, one of the issues I've raised is actually wrong and I acknowledge my mistake. However, my review process is far from being considered "blind", and that was an unfortunate statement of yours.
>
> sorry if you felt that way. i have no intention to attack you.
> "blind" might have been an inappropriate word. maybe "mechanical"?
>
I accept your apologies, but it is important for you to know that either by saying "blind" or "mechanical" you seem to disregard the fact that I've indeed evaluated the scenario before raising the issue. It is not like I had copied some text and pasted it. Otherwise I wouldn't even waste my time reviewing PRs.
> > By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them.
>
> if the code is bug-free, the value doesn't matter.
> they should not be used by anyone.
> thus those NULLify should not be necessary.
The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
And if this single commit naively operates on this dangling pointer, the system will crash.
>
> if there is a bug (i guess there is :-) i prefer it fails loudly.
> that's why i generally don't like the "defensive" methods.
NuttX is embedded in several types of products, from consumer electronics goods to industrial assets. For the majority of applications (if not all of them) the goal is to maximize the mean time between failures.
So, considering this, your preference for "failing loudly" and "fix bugs if any" does not seem much fit.
> i pushed a counter-proposal to the defensive style. 219356c
Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
Now, here it is a simple unit test for making this potential bug evident:
https://godbolt.org/z/f4dzW8M44
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628554049
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
+ return ret;
+ }
+
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ ctx->ws = ws;
}
+ ws = ctx->ws;
+
ninfo("hostname='%s' filename='%s'\n", ws->hostname, ws->filename);
/* The following sequence may repeat indefinitely if we are redirected */
+ conn = &ws->conn;
do
{
- if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
- {
- conn.tls = true;
- }
- else if (!strcmp(ws->scheme, "http"))
+ if (ws->state == WEBCLIENT_STATE_SOCKET)
{
- conn.tls = false;
- }
- else
- {
- nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
- free(ws);
- return -ENOTSUP;
- }
-
- /* Re-initialize portions of the state structure that could have
- * been left from the previous time through the loop and should not
- * persist with the new connection.
- */
-
- ws->httpstatus = HTTPSTATUS_NONE;
- ws->offset = 0;
- ws->datend = 0;
- ws->ndx = 0;
-
- if (conn.tls)
- {
- char port_str[sizeof("65535")];
-
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
+ if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
+ {
+ conn->tls = true;
+ }
+ else if (!strcmp(ws->scheme, "http"))
{
- nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ conn->tls = false;
+ }
+ else
+ {
+ nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
free(ws);
return -ENOTSUP;
}
-#endif
- snprintf(port_str, sizeof(port_str), "%u", ws->port);
- ret = tls_ops->connect(tls_ctx, ws->hostname, port_str,
- CONFIG_WEBCLIENT_TIMEOUT, &conn.tls_conn);
- }
- else
- {
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- struct sockaddr_un server_un;
-#endif
- struct sockaddr_in server_in;
- int domain;
- const struct sockaddr *server_address;
- socklen_t server_address_len;
+ /* Re-initialize portions of the state structure that could have
+ * been left from the previous time through the loop and should not
+ * persist with the new connection.
+ */
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
- {
- domain = PF_LOCAL;
+ ws->httpstatus = HTTPSTATUS_NONE;
+ ws->offset = 0;
+ ws->datend = 0;
+ ws->ndx = 0;
+ ws->redirected = 0;
- memset(&server_un, 0, sizeof(server_un));
- server_un.sun_family = AF_LOCAL;
- strncpy(server_un.sun_path, ctx->unix_socket_path,
- sizeof(server_un.sun_path));
-#if !defined(__NuttX__) && !defined(__linux__)
- server_un.sun_len = SUN_LEN(&server_un);
+ if (conn->tls)
+ {
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ free(ws);
+ return -ENOTSUP;
+ }
#endif
- server_address = (const struct sockaddr *)&server_un;
- server_address_len = sizeof(server_un);
}
else
-#endif
{
- domain = PF_INET;
+ int domain;
+
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ domain = PF_LOCAL;
+ }
+ else
+#endif
+ {
+ domain = PF_INET;
+ }
- /* Get the server address from the host name */
+ /* Create a socket */
- server_in.sin_family = AF_INET;
- server_in.sin_port = htons(ws->port);
- ret = wget_gethostip(ws->hostname, &server_in.sin_addr);
- if (ret < 0)
+ conn->sockfd = socket(domain, SOCK_STREAM, 0);
+ if (conn->sockfd < 0)
{
- /* Could not resolve host (or malformed IP address) */
+ ret = -errno;
+ nerr("ERROR: socket failed: %d\n", errno);
+ goto errout_with_errno;
+ }
- nwarn("WARNING: Failed to resolve hostname\n");
- free(ws);
- return -EHOSTUNREACH;
+ ws->need_conn_close = true;
+
+ if ((ctx->flags & WEBCLIENT_FLAG_NON_BLOCKING) != 0)
+ {
+ int flags = fcntl(conn->sockfd, F_GETFL, 0);
+ ret = fcntl(conn->sockfd, F_SETFL, flags | O_NONBLOCK);
+ if (ret == -1)
+ {
+ ret = -errno;
+ nerr("ERROR: F_SETFL failed: %d\n", ret);
+ goto errout_with_errno;
+ }
}
+ else
+ {
+ /* Set send and receive timeout values */
- server_address = (const struct sockaddr *)&server_in;
- server_address_len = sizeof(struct sockaddr_in);
+ tv.tv_sec = CONFIG_WEBCLIENT_TIMEOUT;
+ tv.tv_usec = 0;
+
+ setsockopt(conn->sockfd, SOL_SOCKET, SO_RCVTIMEO,
+ (FAR const void *)&tv, sizeof(struct timeval));
+ setsockopt(conn->sockfd, SOL_SOCKET, SO_SNDTIMEO,
+ (FAR const void *)&tv, sizeof(struct timeval));
+ }
}
- /* Create a socket */
+ ws->state = WEBCLIENT_STATE_CONNECT;
+ }
- conn.sockfd = socket(domain, SOCK_STREAM, 0);
- if (conn.sockfd < 0)
+ if (ws->state == WEBCLIENT_STATE_CONNECT)
+ {
+ if (conn->tls)
{
- /* socket failed. It will set the errno appropriately */
+ char port_str[sizeof("65535")];
- nerr("ERROR: socket failed: %d\n", errno);
- free(ws);
- return -errno;
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ free(ws);
+ return -ENOTSUP;
+ }
+#endif
+
+ snprintf(port_str, sizeof(port_str), "%u", ws->port);
+ ret = tls_ops->connect(tls_ctx, ws->hostname, port_str,
+ CONFIG_WEBCLIENT_TIMEOUT,
+ &conn->tls_conn);
+ if (ret == 0)
+ {
+ ws->need_conn_close = true;
+ }
}
+ else
+ {
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ struct sockaddr_un server_un;
+#endif
+ struct sockaddr_in server_in;
+ const struct sockaddr *server_address;
+ socklen_t server_address_len;
+
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ memset(&server_un, 0, sizeof(server_un));
+ server_un.sun_family = AF_LOCAL;
+ strncpy(server_un.sun_path, ctx->unix_socket_path,
+ sizeof(server_un.sun_path));
+#if !defined(__NuttX__) && !defined(__linux__)
+ server_un.sun_len = SUN_LEN(&server_un);
+#endif
+ server_address = (const struct sockaddr *)&server_un;
+ server_address_len = sizeof(server_un);
+ }
+ else
+#endif
+ {
+ /* Get the server address from the host name */
- /* Set send and receive timeout values */
+ server_in.sin_family = AF_INET;
+ server_in.sin_port = htons(ws->port);
+ ret = wget_gethostip(ws->hostname, &server_in.sin_addr);
+ if (ret < 0)
+ {
+ /* Could not resolve host (or malformed IP address) */
- tv.tv_sec = CONFIG_WEBCLIENT_TIMEOUT;
- tv.tv_usec = 0;
+ nwarn("WARNING: Failed to resolve hostname\n");
+ free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628552029
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
+ return ret;
+ }
+
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ ctx->ws = ws;
}
+ ws = ctx->ws;
+
ninfo("hostname='%s' filename='%s'\n", ws->hostname, ws->filename);
/* The following sequence may repeat indefinitely if we are redirected */
+ conn = &ws->conn;
do
{
- if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
- {
- conn.tls = true;
- }
- else if (!strcmp(ws->scheme, "http"))
+ if (ws->state == WEBCLIENT_STATE_SOCKET)
{
- conn.tls = false;
- }
- else
- {
- nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
- free(ws);
- return -ENOTSUP;
- }
-
- /* Re-initialize portions of the state structure that could have
- * been left from the previous time through the loop and should not
- * persist with the new connection.
- */
-
- ws->httpstatus = HTTPSTATUS_NONE;
- ws->offset = 0;
- ws->datend = 0;
- ws->ndx = 0;
-
- if (conn.tls)
- {
- char port_str[sizeof("65535")];
-
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
+ if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
+ {
+ conn->tls = true;
+ }
+ else if (!strcmp(ws->scheme, "http"))
{
- nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ conn->tls = false;
+ }
+ else
+ {
+ nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified after `free`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628552751
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
+ return ret;
+ }
+
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ ctx->ws = ws;
}
+ ws = ctx->ws;
+
ninfo("hostname='%s' filename='%s'\n", ws->hostname, ws->filename);
/* The following sequence may repeat indefinitely if we are redirected */
+ conn = &ws->conn;
do
{
- if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
- {
- conn.tls = true;
- }
- else if (!strcmp(ws->scheme, "http"))
+ if (ws->state == WEBCLIENT_STATE_SOCKET)
{
- conn.tls = false;
- }
- else
- {
- nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
- free(ws);
- return -ENOTSUP;
- }
-
- /* Re-initialize portions of the state structure that could have
- * been left from the previous time through the loop and should not
- * persist with the new connection.
- */
-
- ws->httpstatus = HTTPSTATUS_NONE;
- ws->offset = 0;
- ws->datend = 0;
- ws->ndx = 0;
-
- if (conn.tls)
- {
- char port_str[sizeof("65535")];
-
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
+ if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
+ {
+ conn->tls = true;
+ }
+ else if (!strcmp(ws->scheme, "http"))
{
- nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ conn->tls = false;
+ }
+ else
+ {
+ nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
free(ws);
return -ENOTSUP;
}
-#endif
- snprintf(port_str, sizeof(port_str), "%u", ws->port);
- ret = tls_ops->connect(tls_ctx, ws->hostname, port_str,
- CONFIG_WEBCLIENT_TIMEOUT, &conn.tls_conn);
- }
- else
- {
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- struct sockaddr_un server_un;
-#endif
- struct sockaddr_in server_in;
- int domain;
- const struct sockaddr *server_address;
- socklen_t server_address_len;
+ /* Re-initialize portions of the state structure that could have
+ * been left from the previous time through the loop and should not
+ * persist with the new connection.
+ */
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
- {
- domain = PF_LOCAL;
+ ws->httpstatus = HTTPSTATUS_NONE;
+ ws->offset = 0;
+ ws->datend = 0;
+ ws->ndx = 0;
+ ws->redirected = 0;
- memset(&server_un, 0, sizeof(server_un));
- server_un.sun_family = AF_LOCAL;
- strncpy(server_un.sun_path, ctx->unix_socket_path,
- sizeof(server_un.sun_path));
-#if !defined(__NuttX__) && !defined(__linux__)
- server_un.sun_len = SUN_LEN(&server_un);
+ if (conn->tls)
+ {
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ free(ws);
+ return -ENOTSUP;
+ }
#endif
- server_address = (const struct sockaddr *)&server_un;
- server_address_len = sizeof(server_un);
}
else
-#endif
{
- domain = PF_INET;
+ int domain;
+
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ domain = PF_LOCAL;
+ }
+ else
+#endif
+ {
+ domain = PF_INET;
+ }
- /* Get the server address from the host name */
+ /* Create a socket */
- server_in.sin_family = AF_INET;
- server_in.sin_port = htons(ws->port);
- ret = wget_gethostip(ws->hostname, &server_in.sin_addr);
- if (ret < 0)
+ conn->sockfd = socket(domain, SOCK_STREAM, 0);
+ if (conn->sockfd < 0)
{
- /* Could not resolve host (or malformed IP address) */
+ ret = -errno;
+ nerr("ERROR: socket failed: %d\n", errno);
+ goto errout_with_errno;
+ }
- nwarn("WARNING: Failed to resolve hostname\n");
- free(ws);
- return -EHOSTUNREACH;
+ ws->need_conn_close = true;
+
+ if ((ctx->flags & WEBCLIENT_FLAG_NON_BLOCKING) != 0)
+ {
+ int flags = fcntl(conn->sockfd, F_GETFL, 0);
+ ret = fcntl(conn->sockfd, F_SETFL, flags | O_NONBLOCK);
+ if (ret == -1)
+ {
+ ret = -errno;
+ nerr("ERROR: F_SETFL failed: %d\n", ret);
+ goto errout_with_errno;
+ }
}
+ else
+ {
+ /* Set send and receive timeout values */
- server_address = (const struct sockaddr *)&server_in;
- server_address_len = sizeof(struct sockaddr_in);
+ tv.tv_sec = CONFIG_WEBCLIENT_TIMEOUT;
+ tv.tv_usec = 0;
+
+ setsockopt(conn->sockfd, SOL_SOCKET, SO_RCVTIMEO,
+ (FAR const void *)&tv, sizeof(struct timeval));
+ setsockopt(conn->sockfd, SOL_SOCKET, SO_SNDTIMEO,
+ (FAR const void *)&tv, sizeof(struct timeval));
+ }
}
- /* Create a socket */
+ ws->state = WEBCLIENT_STATE_CONNECT;
+ }
- conn.sockfd = socket(domain, SOCK_STREAM, 0);
- if (conn.sockfd < 0)
+ if (ws->state == WEBCLIENT_STATE_CONNECT)
+ {
+ if (conn->tls)
{
- /* socket failed. It will set the errno appropriately */
+ char port_str[sizeof("65535")];
- nerr("ERROR: socket failed: %d\n", errno);
- free(ws);
- return -errno;
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] mlyszczek commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r632188065
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
Review comment:
@yamt
Ah yes, didn't look close enough, my bad. Yeah I think in this particular case nulling ctx->ws seems like a good idea. But I didn't check it good enough to be sure if that is necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839761694
> > > if the code is bug-free, the value doesn't matter.
> > > they should not be used by anyone.
> > > thus those NULLify should not be necessary.
> >
> >
> > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> > And if this single commit naively operates on this dangling pointer, the system will crash.
>
> why doesn't a local variable matter?
> with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases where the value does matter.
If a local variable goes out-of-scope (i.e. its lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
> > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> >
> >
> > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > Now, here it is a simple unit test for making this potential bug evident:
> > https://godbolt.org/z/f4dzW8M44
>
> the unit test is just broken.
> it doesn't seem to support your argument.
Guess why it is broken?
```
Program stderr
free(): double free detected in tcache 2
```
I rest my case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-835000482
> > in my view:
> > assign only when necessary. fix bugs if any. -> strict coding
> > make unnecessary assignments to hide potential bugs. -> loose coding
>
> Sorry, I cannot agree with your view.
no problem with having different views.
actually it's a good thing of a community.
> It is not unnecessary when the motivation for it is to prevent (not hide) a potential bug. And depending on how this opensource code is integrated into a product, this potential bug could escalate into a security vulnerability.
the "defensive" style can make it difficult to find such bugs.
i know some people like constructs like the following.
while i can understand the motivation, my honest impression is that using C was
a bad idea for those people in the first place.
```
#define FREE(a) (free(a); a = NULL)
#define MALLOC(a) calloc(1, a)
```
>
> > but in this particular case, i don't feel it's an improvement.
>
> It is not an improvement, this is the recommended method for avoiding **use-after-free** and **double-free** errors, both listed in the CWE:
> https://cwe.mitre.org/data/definitions/415.html
> https://cwe.mitre.org/data/definitions/416.html
i certainly don't recommend to apply the method blindly.
(i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-840377859
> > > > if the code is bug-free, the value doesn't matter.
> > > > they should not be used by anyone.
> > > > thus those NULLify should not be necessary.
> > >
> > >
> > > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> > > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> > > And if this single commit naively operates on this dangling pointer, the system will crash.
> >
> >
> > why doesn't a local variable matter?
> > with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
> Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases where the value does matter.
> If a local variable goes out-of-scope (i.e. its lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
buggy software can happily access the value by reading an uninitialized variable.
anyway, i'm not a fan of nullify.
>
> > > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> > >
> > >
> > > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > > Now, here it is a simple unit test for making this potential bug evident:
> > > https://godbolt.org/z/f4dzW8M44
> >
> >
> > the unit test is just broken.
> > it doesn't seem to support your argument.
>
> Guess why it is broken?
>
> ```
> Program stderr
> free(): double free detected in tcache 2
> ```
>
> I rest my case.
it's something like having a unit test like the following and claiming that free() has a problem.
it doesn't make much sense.
```
p = malloc(1);
free(p);
free(p);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839761694
> > > if the code is bug-free, the value doesn't matter.
> > > they should not be used by anyone.
> > > thus those NULLify should not be necessary.
> >
> >
> > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> > And if this single commit naively operates on this dangling pointer, the system will crash.
>
> why doesn't a local variable matter?
> with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases where the value does matter.
If a local variable goes out-of-scope (i.e. it's lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
> > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> >
> >
> > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > Now, here it is a simple unit test for making this potential bug evident:
> > https://godbolt.org/z/f4dzW8M44
>
> the unit test is just broken.
> it doesn't seem to support your argument.
Oh, really? Guess why it is broken?
```
Program stderr
free(): double free detected in tcache 2
```
I rest my case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-840591927
> > Well, I was referring to local variables that are about to go out-of-scope, that's why I mentioned the extended lifetime for the other cases where the value does matter.
> > If a local variable goes out-of-scope (i.e. its lifetime is over), it cannot be referenced anymore, so its pointer value is harmless. But I won't bother if you still want to nullify it.
>
> buggy software can happily access the value by reading an uninitialized variable.
We are not dealing with "uninitialized variables" here.
And no, the compiler will happily emit an error if the software tries to reference a variable that went out-of-scope.
> anyway, i'm not a fan of nullify.
>
You are completely free to have your personal preferences, but for employing them on an open-source project you are subject to a review process.
And so far, on my judgement, your argumentation is not reasonable.
> > > > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
> > > >
> > > >
> > > > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> > > > Now, here it is a simple unit test for making this potential bug evident:
> > > > https://godbolt.org/z/f4dzW8M44
> > >
> > >
> > > the unit test is just broken.
> > > it doesn't seem to support your argument.
> >
> >
> > Guess why it is broken?
> > ```
> > Program stderr
> > free(): double free detected in tcache 2
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I rest my case.
>
> it's something like having a unit test like the following and claiming that free() has a problem.
> it doesn't make much sense.
>
> ```
> p = malloc(1);
> free(p);
> free(p);
> ```
It's completely different.
If your comparison is based on the fact that I've called the `webclient_abort` twice, feel free to create another test that calls `webclient_perform` after a single `webclient_abort`.
The check here https://github.com/apache/incubator-nuttx-apps/blob/02bf1583d55cc8d0161866ee40c86cee9b97af46/netutils/webclient/webclient.c#L696 will not pass because you did not nullify `ctx->ws` and from https://github.com/apache/incubator-nuttx-apps/blob/02bf1583d55cc8d0161866ee40c86cee9b97af46/netutils/webclient/webclient.c#L723 onwards is undefined behavior because `ctx->ws` points to an already freed memory region.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-829693389
@gustavonihei i fixed some of style issues you pointed out. i'm not going to fix the rest of them in this PR because they are in the existing code, not this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-874099283
> @gustavonihei is the nullification thing the only thing you are unhappy with this PR?
Yes.
Regarding the styling issues, I am fine with doing it in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-840969077
> > > it's something like having a unit test like the following and claiming that free() has a problem.
> > > it doesn't make much sense.
> > > ```
> > > p = malloc(1);
> > > free(p);
> > > free(p);
> > > ```
> >
> >
> > It's completely different.
> > If your comparison is based on the fact that I've called the `webclient_abort` twice, feel free to create another test that calls `webclient_perform` after a single `webclient_abort`.
>
> it's same.
>
> webclient_perform after webclient_abort is also same.
> they are all wrong use of apis.
>
> you can invent more and more "problematic" sequences. eg. realloc after free.
> but it doesn't mean these apis has problems.
i added a dedicated comment block to explain webclient_context lifetime
to explain the expected sequence of API calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r622259139
##########
File path: include/netutils/webclient.h
##########
@@ -249,6 +277,19 @@ struct webclient_context
unsigned int http_status;
FAR char *http_reason;
size_t http_reason_len;
+
+ struct wget_s *ws;
+};
+
+struct webclient_poll_info
+{
+ /* A file descriptor to wait for i/o. */
+
+ int fd;
+ unsigned int flags; /* OR'ed WEBCLIENT_POLL_INFO_xxx flags */
+
+#define WEBCLIENT_POLL_INFO_WANT_READ 1U
+#define WEBCLIENT_POLL_INFO_WANT_WRITE 2U
Review comment:
Placement of these preprocessor definitions is not compliant to the coding standard.
##########
File path: netutils/webclient/webclient.c
##########
@@ -132,6 +129,37 @@
* Private Types
****************************************************************************/
+enum webclient_state
+ {
+ WEBCLIENT_STATE_SOCKET,
+ WEBCLIENT_STATE_CONNECT,
+ WEBCLIENT_STATE_PREPARE_REQUEST,
+ WEBCLIENT_STATE_SEND_REQUEST,
+ WEBCLIENT_STATE_SEND_REQUEST_BODY,
+ WEBCLIENT_STATE_STATUSLINE,
+ WEBCLIENT_STATE_HEADERS,
+ WEBCLIENT_STATE_DATA,
+ WEBCLIENT_STATE_CLOSE,
+ WEBCLIENT_STATE_DONE,
+ };
+
+struct conn
+{
+ bool tls;
+
+ /* for !tls */
+
+ int sockfd;
+ unsigned int flags;
+
+#define CONN_WANT_READ WEBCLIENT_POLL_INFO_WANT_READ
+#define CONN_WANT_WRITE WEBCLIENT_POLL_INFO_WANT_WRITE
Review comment:
Placement of these preprocessor definitions is not compliant to the coding standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-834918654
> @gustavonihei
> @acassis
> i know some people like those construct.
> but i tend to think it "loose", not "defensive".
Sorry, I couldn't follow you. What do you mean by "loose"? Could you please expand on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-834929878
> > @gustavonihei
> > @acassis
> > i know some people like those construct.
> > but i tend to think it "loose", not "defensive".
>
> Sorry, I couldn't follow you. What do you mean by "loose"? Could you please expand on this?
in my view:
assign only when necessary. fix bugs if any. -> strict coding
make unnecessary assignments to hide potential bugs. -> loose coding
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628552382
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
+ return ret;
+ }
+
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ ctx->ws = ws;
}
+ ws = ctx->ws;
+
ninfo("hostname='%s' filename='%s'\n", ws->hostname, ws->filename);
/* The following sequence may repeat indefinitely if we are redirected */
+ conn = &ws->conn;
do
{
- if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
- {
- conn.tls = true;
- }
- else if (!strcmp(ws->scheme, "http"))
+ if (ws->state == WEBCLIENT_STATE_SOCKET)
{
- conn.tls = false;
- }
- else
- {
- nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
- free(ws);
- return -ENOTSUP;
- }
-
- /* Re-initialize portions of the state structure that could have
- * been left from the previous time through the loop and should not
- * persist with the new connection.
- */
-
- ws->httpstatus = HTTPSTATUS_NONE;
- ws->offset = 0;
- ws->datend = 0;
- ws->ndx = 0;
-
- if (conn.tls)
- {
- char port_str[sizeof("65535")];
-
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
+ if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
+ {
+ conn->tls = true;
+ }
+ else if (!strcmp(ws->scheme, "http"))
{
- nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ conn->tls = false;
+ }
+ else
+ {
+ nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
free(ws);
return -ENOTSUP;
}
-#endif
- snprintf(port_str, sizeof(port_str), "%u", ws->port);
- ret = tls_ops->connect(tls_ctx, ws->hostname, port_str,
- CONFIG_WEBCLIENT_TIMEOUT, &conn.tls_conn);
- }
- else
- {
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- struct sockaddr_un server_un;
-#endif
- struct sockaddr_in server_in;
- int domain;
- const struct sockaddr *server_address;
- socklen_t server_address_len;
+ /* Re-initialize portions of the state structure that could have
+ * been left from the previous time through the loop and should not
+ * persist with the new connection.
+ */
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
- {
- domain = PF_LOCAL;
+ ws->httpstatus = HTTPSTATUS_NONE;
+ ws->offset = 0;
+ ws->datend = 0;
+ ws->ndx = 0;
+ ws->redirected = 0;
- memset(&server_un, 0, sizeof(server_un));
- server_un.sun_family = AF_LOCAL;
- strncpy(server_un.sun_path, ctx->unix_socket_path,
- sizeof(server_un.sun_path));
-#if !defined(__NuttX__) && !defined(__linux__)
- server_un.sun_len = SUN_LEN(&server_un);
+ if (conn->tls)
+ {
+#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
+ if (ctx->unix_socket_path != NULL)
+ {
+ nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
+ return ret;
+ }
+
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ ctx->ws = ws;
}
+ ws = ctx->ws;
+
ninfo("hostname='%s' filename='%s'\n", ws->hostname, ws->filename);
/* The following sequence may repeat indefinitely if we are redirected */
+ conn = &ws->conn;
do
{
- if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
- {
- conn.tls = true;
- }
- else if (!strcmp(ws->scheme, "http"))
+ if (ws->state == WEBCLIENT_STATE_SOCKET)
{
- conn.tls = false;
- }
- else
- {
- nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
- free(ws);
- return -ENOTSUP;
- }
-
- /* Re-initialize portions of the state structure that could have
- * been left from the previous time through the loop and should not
- * persist with the new connection.
- */
-
- ws->httpstatus = HTTPSTATUS_NONE;
- ws->offset = 0;
- ws->datend = 0;
- ws->ndx = 0;
-
- if (conn.tls)
- {
- char port_str[sizeof("65535")];
-
-#if defined(CONFIG_WEBCLIENT_NET_LOCAL)
- if (ctx->unix_socket_path != NULL)
+ if (!strcmp(ws->scheme, "https") && tls_ops != NULL)
+ {
+ conn->tls = true;
+ }
+ else if (!strcmp(ws->scheme, "http"))
{
- nerr("ERROR: TLS on AF_LOCAL socket is not implemented\n");
+ conn->tls = false;
+ }
+ else
+ {
+ nerr("ERROR: unsupported scheme: %s\n", ws->scheme);
free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-835642332
> the "defensive" style can make it difficult to find such bugs.
@yamt It doesn't make sense. If you are following the recommended mitigation methods and therefore preventing these bugs from happening, which bugs are you expecting to find then?
> i certainly don't recommend to apply the method blindly.
> (i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.)
Indeed, one of the issues I've raised is actually wrong and I acknowledge my mistake. However, my review process is far from being considered "blind", and that was an unfortunate statement of yours.
By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628592733
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
Review comment:
are you saying that there is a bug?
or is it just about a coding style?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628553765
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] mlyszczek commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
mlyszczek commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r632183250
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
Review comment:
This is kinda pointless in situations like this. Dangling pointers are a thing if we are talking about global variables or library objects that are passed to library functions. But in this case? 1) return is called every time free() is called - thus ws is lost anyway 2) ws is accessed in this function directly via ws->, so even if you nullify this, you will still crash when calling ws->status or dereference ws in any way. Nullifying local variables before return is superflous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-874099283
> @gustavonihei is the nullification thing the only thing you are unhappy with this PR?
Yes.
Regarding the styling issues, I am fine with doing it in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r622255657
##########
File path: include/netutils/webclient.h
##########
@@ -165,6 +165,7 @@ typedef CODE int (*webclient_body_callback_t)(
FAR void *ctx);
struct webclient_tls_connection;
+struct webclient_poll_info;
Review comment:
```suggestion
struct webclient_tls_connection_s;
struct webclient_poll_info_s;
```
According to the coding style, structure names must end with the suffix `_s`.
There are other occurrences in this header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839347064
> > > > the "defensive" style can make it difficult to find such bugs.
> > >
> > >
> > > @yamt It doesn't make sense. If you are following the recommended mitigation methods and therefore preventing these bugs from happening, which bugs are you expecting to find then?
> >
> >
> > unintended uses of values.
>
> Fine. So as long the recommended mitigation methods are being followed, you won't find them. Better safe than sorry.
i don't understand your logic here.
>
> > > > i certainly don't recommend to apply the method blindly.
> > > > (i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.)
> > >
> > >
> > > Indeed, one of the issues I've raised is actually wrong and I acknowledge my mistake. However, my review process is far from being considered "blind", and that was an unfortunate statement of yours.
> >
> >
> > sorry if you felt that way. i have no intention to attack you.
> > "blind" might have been an inappropriate word. maybe "mechanical"?
>
> I accept your apologies, but it is important for you to know that either by saying "blind" or "mechanical" you seem to disregard the fact that I've indeed evaluated the scenario before raising the issue. It is not like I had copied some text and pasted it. Otherwise I wouldn't even waste my time reviewing PRs.
ok.
>
> > > By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them.
> >
> >
> > if the code is bug-free, the value doesn't matter.
> > they should not be used by anyone.
> > thus those NULLify should not be necessary.
>
> The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter.
> And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit.
> And if this single commit naively operates on this dangling pointer, the system will crash.
why doesn't a local variable matter?
with your logic, i think the value left on stack might be unintentionally used by a buggy software.
>
> > if there is a bug (i guess there is :-) i prefer it fails loudly.
> > that's why i generally don't like the "defensive" methods.
>
> NuttX is embedded in several types of products, from consumer electronics goods to industrial assets. For the majority of applications (if not all of them) the goal is to maximize the mean time between failures.
> So, considering this, your preference for "failing loudly" and "fix bugs if any" does not seem much fit.
i don't agree.
running with a wrong state is often worse than a crash. it depends.
>
> > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9)
>
> Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised.
> Now, here it is a simple unit test for making this potential bug evident:
> https://godbolt.org/z/f4dzW8M44
the unit test is just broken.
it doesn't seem to support your argument.
(btw, the website looks cool.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-834962657
> in my view:
> assign only when necessary. fix bugs if any. -> strict coding
> make unnecessary assignments to hide potential bugs. -> loose coding
Sorry, I cannot agree with your view.
It is not unnecessary when the motivation for it is to prevent (not hide) a potential bug. And depending on how this opensource code is integrated into a product, this potential bug could escalate into a security vulnerability.
> but in this particular case, i don't feel it's an improvement.
It is not an improvement, this is the recommended method for avoiding **use-after-free** and **double-free** errors, both listed in the CWE:
https://cwe.mitre.org/data/definitions/415.html
https://cwe.mitre.org/data/definitions/416.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-892370531
i marked this a draft because i haven't tested after the last rebase, which was not trivial.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei edited a comment on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-874099283
> @gustavonihei is the nullification thing the only thing you are unhappy with this PR?
Yes.
Regarding the styling issues, I am fine with addressing them in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-834945638
please don't get me wrong. being loose/defensive is not necessarily a bad thing. actually i myself sometimes do that. but in this particular case, i don't feel it's an improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628554252
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
Review comment:
```suggestion
free(ws);
ctx->ws = NULL;
```
To avoid use-after-free errors, the freed pointer should be nullified.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-834908004
@gustavonihei
@acassis
i know some people like those construct.
but i tend to think it "loose", not "defensive".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628631483
##########
File path: netutils/webclient/webclient.c
##########
@@ -910,131 +1040,184 @@ int webclient_perform(FAR struct webclient_context *ctx)
goto errout_with_errno;
}
- ret = conn_send_all(ctx, &conn, input_buffer,
- input_buffer_size);
- if (ret < 0)
+ ninfo("Got %zu bytes body chunk / total remaining %zu bytes\n",
+ input_buffer_size, ws->state_len);
+ ws->data_buffer = input_buffer;
+ ws->data_len = input_buffer_size;
+ ws->state_offset = 0;
+ }
+ else
+ {
+ size_t bytes_to_send = ws->data_len - ws->state_offset;
+
+ DEBUGASSERT(bytes_to_send <= ws->state_len);
+ ssize_t ssz = conn_send(ctx, conn,
+ ws->data_buffer + ws->state_offset,
+ bytes_to_send);
+ if (ssz < 0)
{
- break;
+ ret = ssz;
+ nerr("ERROR: send failed: %d\n", -ret);
+ goto errout_with_errno;
}
- sent += input_buffer_size;
+ ninfo("Sent %zd bytes request body at offset %ju "
+ "in the %zu bytes chunk, "
+ "total remaining %zu bytes\n",
+ ssz,
+ (uintmax_t)ws->state_offset,
+ ws->data_len,
+ ws->state_len);
+ ws->state_len -= ssz;
+ ws->state_offset += ssz;
+ DEBUGASSERT(ws->state_offset <= ws->data_len);
+ if (ws->state_offset == ws->data_len)
+ {
+ ws->data_buffer = NULL;
+ }
}
}
- if (ret < 0)
- {
- nerr("ERROR: send failed: %d\n", -ret);
- goto errout_with_errno;
- }
-
/* Now loop to get the file sent in response to the GET. This
* loop continues until either we read the end of file (nbytes == 0)
* or until we detect that we have been redirected.
*/
- ws->state = WEBCLIENT_STATE_STATUSLINE;
- redirected = false;
- for (; ; )
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE ||
+ ws->state == WEBCLIENT_STATE_HEADERS ||
+ ws->state == WEBCLIENT_STATE_DATA)
{
- ws->datend = conn_recv(ctx, &conn, ws->buffer, ws->buflen);
- if (ws->datend < 0)
+ for (; ; )
{
- ret = ws->datend;
- nerr("ERROR: recv failed: %d\n", -ret);
- goto errout_with_errno;
- }
- else if (ws->datend == 0)
- {
- if (ws->state != WEBCLIENT_STATE_DATA)
+ ws->datend = conn_recv(ctx, conn, ws->buffer, ws->buflen);
+ if (ws->datend < 0)
{
- nerr("Connection lost unexpectedly\n");
- ret = -ECONNABORTED;
+ ret = ws->datend;
+ nerr("ERROR: recv failed: %d\n", -ret);
goto errout_with_errno;
}
+ else if (ws->datend == 0)
+ {
+ if (ws->state != WEBCLIENT_STATE_DATA)
+ {
+ nerr("Connection lost unexpectedly\n");
+ ret = -ECONNABORTED;
+ goto errout_with_errno;
+ }
- ninfo("Connection lost\n");
- break;
- }
+ ninfo("Connection lost\n");
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 0;
+ break;
+ }
- /* Handle initial parsing of the status line */
+ /* Handle initial parsing of the status line */
- ws->offset = 0;
- if (ws->state == WEBCLIENT_STATE_STATUSLINE)
- {
- ret = wget_parsestatus(ctx, ws);
- if (ret < 0)
+ ws->offset = 0;
+ if (ws->state == WEBCLIENT_STATE_STATUSLINE)
{
- goto errout_with_errno;
+ ret = wget_parsestatus(ctx, ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Parse the HTTP data */
+ /* Parse the HTTP data */
- if (ws->state == WEBCLIENT_STATE_HEADERS)
- {
- ret = wget_parseheaders(ws);
- if (ret < 0)
+ if (ws->state == WEBCLIENT_STATE_HEADERS)
{
- goto errout_with_errno;
+ ret = wget_parseheaders(ws);
+ if (ret < 0)
+ {
+ goto errout_with_errno;
+ }
}
- }
- /* Dispose of the data payload */
+ /* Dispose of the data payload */
- if (ws->state == WEBCLIENT_STATE_DATA)
- {
- if (ws->httpstatus != HTTPSTATUS_MOVED)
+ if (ws->state == WEBCLIENT_STATE_DATA)
{
- /* Let the client decide what to do with the
- * received file.
- */
-
- if (ws->offset == ws->datend)
+ if (ws->httpstatus != HTTPSTATUS_MOVED)
{
- /* We don't have data to give to the client yet. */
- }
- else if (ctx->sink_callback)
- {
- ret = ctx->sink_callback(&ws->buffer, ws->offset,
- ws->datend, &ws->buflen,
- ctx->sink_callback_arg);
- if (ret != 0)
+ /* Let the client decide what to do with the
+ * received file.
+ */
+
+ if (ws->offset == ws->datend)
{
- goto errout_with_errno;
+ /* We don't have data to give to the client yet. */
+ }
+ else if (ctx->sink_callback)
+ {
+ ret = ctx->sink_callback(&ws->buffer, ws->offset,
+ ws->datend, &ws->buflen,
+ ctx->sink_callback_arg);
+ if (ret != 0)
+ {
+ goto errout_with_errno;
+ }
+ }
+ else
+ {
+ ctx->callback(&ws->buffer, ws->offset, ws->datend,
+ &ws->buflen, ctx->sink_callback_arg);
}
}
else
{
- ctx->callback(&ws->buffer, ws->offset, ws->datend,
- &ws->buflen, ctx->sink_callback_arg);
- }
- }
- else
- {
- redirected = true;
- nredirect++;
- if (nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
- {
- nerr("ERROR: too many redirects (%u)\n", nredirect);
- goto errout;
- }
+ ws->nredirect++;
+ if (ws->nredirect > CONFIG_WEBCLIENT_MAX_REDIRECT)
+ {
+ nerr("ERROR: too many redirects (%u)\n",
+ ws->nredirect);
+ ret = -ELOOP;
+ goto errout_with_errno;
+ }
- break;
+ ws->state = WEBCLIENT_STATE_CLOSE;
+ ws->redirected = 1;
+ break;
+ }
}
}
}
- conn_close(ctx, &conn);
+ if (ws->state == WEBCLIENT_STATE_CLOSE)
+ {
+ conn_close(ctx, conn);
+ ws->need_conn_close = false;
+ if (ws->redirected)
+ {
+ ws->state = WEBCLIENT_STATE_SOCKET;
+ }
+ else
+ {
+ ws->state = WEBCLIENT_STATE_DONE;
+ }
+ }
}
- while (redirected);
+ while (ws->state != WEBCLIENT_STATE_DONE);
free(ws);
Review comment:
You may call it **defensive coding**.
Even after `free`, the `ctx->ws` pointer will still point to a memory address and since it is a member of a context structure it won't go out-of-scope. `ctx->ws` is now called a **dangling pointer**.
So every other check against `NULL` is simply useless if `ctx->ws` is not nullified, making the software prone to _user-after-free_ and _double-free_ kinds of errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] gustavonihei commented on a change in pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#discussion_r628816029
##########
File path: netutils/webclient/webclient.c
##########
@@ -674,225 +693,336 @@ int webclient_perform(FAR struct webclient_context *ctx)
/* Initialize the state structure */
- ws = calloc(1, sizeof(struct wget_s));
- if (!ws)
+ if (ctx->ws == NULL)
{
- return -errno;
- }
+ ws = calloc(1, sizeof(struct wget_s));
+ if (!ws)
+ {
+ return -errno;
+ }
- ws->buffer = ctx->buffer;
- ws->buflen = ctx->buflen;
+ ws->buffer = ctx->buffer;
+ ws->buflen = ctx->buflen;
- /* Parse the hostname (with optional port number) and filename
- * from the URL.
- */
+ /* Parse the hostname (with optional port number) and filename
+ * from the URL.
+ */
- ret = parseurl(ctx->url, ws);
- if (ret != 0)
- {
- nwarn("WARNING: Malformed URL: %s\n", ctx->url);
- free(ws);
- return ret;
+ ret = parseurl(ctx->url, ws);
+ if (ret != 0)
+ {
+ nwarn("WARNING: Malformed URL: %s\n", ctx->url);
+ free(ws);
Review comment:
This is one is not actually an issue since it `ctx->ws` is already NULL due to checking on line 696.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] yamt commented on pull request #695: webclient: Implement non-blocking I/O
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #695:
URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-841904553
the last push was a conflict resolution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org