[PATCH ncns v1 0/2] ncns fixes

[PATCH ncns v1 0/2] ncns fixes

[Ammar Faizi]

Hi Al,

Yet another small series for ncns.

1) Initialize `read_ret_` to zero.

We can't rely on read_ret_ value being random at initialization. This
caused a real crash where @ret value being random (83382480 in this
case):

  ret = 83382480
  [0817/093957.227555:FATAL:scoped_refptr.h(272)] Check failed: ptr_.
  #0 0x7fa9a8a6b62c <unknown>
  #1 0x7fa9a87cb30a <unknown>
  #2 0x7fa9a87cb2c5 <unknown>
  #3 0x7fa9a881c799 <unknown>
  #4 0x7fa9a881cfa9 <unknown>
  #5 0x7fa9a878537b <unknown>
  #6 0x7fa9a93866b6 <unknown>
  #7 0x7fa9a93121fa <unknown>
  #8 0x7fa9a931debd <unknown>
  #9 0x000000b0deed <unknown>
  #10 0x000000d972d0 <unknown>
  #11 0x000000d9880f <unknown>
  #12 0x0000016dabb9 <unknown>

2) Set error string when the Chromium hits net error.

Translate the error code to string when the Chromium hits a net error.
This makes debugging easier for the callers because now we can know
what the returned error code means, easily.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
Ammar Faizi (2):
  chnet: Initialize `read_ret_` to zero
  chnet: Set error string when the Chromium hits net error

 chnet/chnet.cc | 12 ++++++++++++
 chnet/chnet.h  |  2 ++
 2 files changed, 14 insertions(+)


base-commit: 971f49f53c730aa82466b251a2b682c0827c3ec4
-- 
Ammar Faizi

[PATCH ncns v1 1/2] chnet: Initialize `read_ret_` to zero

[Ammar Faizi]

We can't rely on read_ret_ value being random at initialization. This
caused a real crash where @ret value being random (83382480 in this
case):

  ret = 83382480
  [0817/093957.227555:FATAL:scoped_refptr.h(272)] Check failed: ptr_.
  #0 0x7fa9a8a6b62c <unknown>
  #1 0x7fa9a87cb30a <unknown>
  #2 0x7fa9a87cb2c5 <unknown>
  #3 0x7fa9a881c799 <unknown>
  #4 0x7fa9a881cfa9 <unknown>
  #5 0x7fa9a878537b <unknown>
  #6 0x7fa9a93866b6 <unknown>
  #7 0x7fa9a93121fa <unknown>
  #8 0x7fa9a931debd <unknown>
  #9 0x000000b0deed <unknown>
  #10 0x000000d972d0 <unknown>
  #11 0x000000d9880f <unknown>
  #12 0x0000016dabb9 <unknown>

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 chnet/chnet.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index 37c0b03..c870e05 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -43,6 +43,7 @@ CHNetDelegate::CHNetDelegate(void):
 {
 	base::Thread::Options options(base::MessagePumpType::IO, 0);
 	CHECK(thread_.StartWithOptions(std::move(options)));
+	read_ret_.store(0, std::memory_order_relaxed);
 }
 
 static void CHNetDelegateDestruct(std::unique_ptr<URLRequest> *url_req,
-- 
Ammar Faizi

Re: [PATCH ncns v1 1/2] chnet: Initialize `read_ret_` to zero

[Alviro Iskandar Setiawan]

On Wed, Aug 17, 2022 at 8:35 PM Ammar Faizi wrote:
> We can't rely on read_ret_ value being random at initialization. This
> caused a real crash where @ret value being random (83382480 in this
> case):

good catch btw, tq for the fix

tq

-- Viro

[PATCH ncns v1 2/2] chnet: Set error string when the Chromium hits net error

[Ammar Faizi]

Translate the error code to string when the Chromium hits a net error.
This makes debugging easier for the callers because now we can know
what the returned error code means, easily.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 chnet/chnet.cc | 11 +++++++++++
 chnet/chnet.h  |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index c870e05..9d19882 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -149,6 +149,9 @@ int CHNetDelegate::Read(int size)
 
 void CHNetDelegate::OnResponseStarted(URLRequest *url_req, int net_err)
 {
+	if (unlikely(net_err < 0))
+		SetError(net_err);
+
 	if (cb.response_started_)
 		cb.response_started_(cb.response_started_data_, url_req,
 				     net_err);
@@ -156,11 +159,19 @@ void CHNetDelegate::OnResponseStarted(URLRequest *url_req, int net_err)
 
 void CHNetDelegate::OnReadCompleted(URLRequest *url_req, int bytes_read)
 {
+	if (unlikely(bytes_read < 0))
+		SetError(bytes_read);
+
 	read_ret_.store(bytes_read);
 	if (cb.read_completed_)
 		cb.read_completed_(cb.read_completed_data_, url_req, bytes_read);
 }
 
+void CHNetDelegate::SetError(int err_code)
+{
+	err_ = "chromium_net_err:" + ErrorToString(err_code);
+}
+
 } /* namespace net */
 
 CHNet::CHNet(void)
diff --git a/chnet/chnet.h b/chnet/chnet.h
index f91b25b..15ba8cd 100644
--- a/chnet/chnet.h
+++ b/chnet/chnet.h
@@ -26,6 +26,7 @@
 #include "net/url_request/url_request_context_builder.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request.h"
+#include "net/base/net_errors.h"
 #define CHNET_EXPORT COMPONENT_EXPORT(CHNET)
 
 #else /* #ifdef __FOR_CHROMIUM_INTERNAL */
@@ -73,6 +74,7 @@ public:
 	int Start(void);
 	int Read(int size);
 	void _Read(Waiter *sig, int size);
+	void SetError(int err_code);
 
 	void OnResponseStarted(URLRequest *url_req, int net_error) override;
 	void OnReadCompleted(URLRequest *url_req, int bytes_read) override;
-- 
Ammar Faizi

Re: [PATCH ncns v1 0/2] ncns fixes

[Alviro Iskandar Setiawan]

On Wed, Aug 17, 2022 at 8:35 PM Ammar Faizi wrote:
> Hi Al,
>
> Yet another small series for ncns.

applied

tq

-- Viro

[PATCH ncns v1 0/2] ncns fixes

[Ammar Faizi]

Hi Al,

Two fixes this time:

1) Only take the pointer of buffer when available.

The call to `ch->ch_.read_buf()` may be undefined behavior if the
buffer hasn't been initialized. Don't touch this buffer and return
null if it's not guaranteed to be initialized.

2) Fix unused variable warning.

Fix this:

  ../chnet/chnet_node.cc: In function ‘void CHN_RingWaitCQE(const Napi::CallbackInfo&)’:
  ../chnet/chnet_node.cc:146:19: warning: variable ‘env’ set but not used [-Wunused-but-set-variable]
    146 |         Napi::Env env = info.Env();
        |                   ^~~

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  chnet: node: Only take the pointer of buffer when available
  chnet: node: Fix unused variable warning

 chnet/chnet_node.cc | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)


base-commit: 2b40ff58ef9007013ec741d2d599869e4c0b46f2
-- 
Ammar Faizi

Re: [PATCH ncns v1 0/2] ncns fixes

[Alviro Iskandar Setiawan]

On Wed, Aug 17, 2022 at 8:09 AM Ammar Faizi wrote:
> Hi Al,
>
> Two fixes this time:

applied

tq

-- Viro