[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