Brave-browser: How to proceed with patch for MDNS fix

[RangerMauve]

Description

I'm currently working on fixing the MDNS peer discovery issue in ipfs-companion.

In order to fix this I discovered that we needed to change Chrome's UDP socket API to have a flag to enable address reuse which is the default in Node.js's UDP API.

To that end, I've added a new api called chrome.sockets.udp.setAllowAddressReuse which is inspired by this chromium patch from 2017.

I've got it working and it's fixing the issue, but I'm unsure as to how to proceed with submitting the patch to Brave.

@lidel linked me to this guide and I was thinking of using the preprocessor method in order to add the lines I needed in brave-core/chromium_src/, but I'm not sure if that's the best way forward.

Some guidance on what the best approach would be to get this patch accepted would be very much appreciated.

Steps to Reproduce

1. Run IPFS-companion on two brave instances
2. Disable all transports except MDNS
3. Try to get them to connect

Actual result:

There's an error in the console complaining about address reuse

Expected result:

They should connect

Reproduces how often:

n/a

Brave version (brave://version info)

n/a

Version/Channel Information:

n/a

• Can you reproduce this issue with the current release?
• Can you reproduce this issue with the beta channel?
• Can you reproduce this issue with the dev channel?
• Can you reproduce this issue with the nightly channel?

Other Additional Information:

• Does the issue resolve itself when disabling Brave Shields?
• Does the issue resolve itself when disabling Brave Rewards?
• Is the issue reproducible on the latest version of Chrome?

Miscellaneous Information:

The changes mostly affect the extensions/ folder of the SRC, and I'm a bit worried about the change that was needed in extensions/browser/extension_function_histogram_value.h since it seems values there shouldn't conflict with chromium.

[yrliou]

@RangerMauve Could you share the patch here first so we could take a look to see how to minimize the patching? I suppose yours are different from the 2017 one.

[RangerMauve]

Hey! Here's the git diff for my patch.

It's a little different, but it's not a lot of code overall, I tried to follow the style conventions of the files I modified.

diff --git a/extensions/browser/api/socket/udp_socket.cc b/extensions/browser/api/socket/udp_socket.cc
index 0f129a64cbb9..46819d3165b8 100644
--- a/extensions/browser/api/socket/udp_socket.cc
+++ b/extensions/browser/api/socket/udp_socket.cc
@@ -74,6 +74,7 @@ void UDPSocket::Bind(const std::string& address,
     std::move(callback).Run(net::ERR_INVALID_ARGUMENT);
     return;
   }
+
   socket_->Bind(ip_end_point, std::move(socket_options_),
                 base::BindOnce(&UDPSocket::OnBindCompleted,
                                base::Unretained(this), std::move(callback)));
@@ -381,6 +382,13 @@ void UDPSocket::SetBroadcast(bool enabled,
   socket_->SetBroadcast(enabled, std::move(callback));
 }

+int UDPSocket::SetAllowAddressReuse(bool allowed) {
+  if (!socket_options_)
+    return net::ERR_SOCKET_IS_CONNECTED;
+  socket_options_->allow_address_reuse = allowed;
+  return net::OK;
+}
+
 const std::vector<std::string>& UDPSocket::GetJoinedGroups() const {
   return multicast_groups_;
 }
diff --git a/extensions/browser/api/socket/udp_socket.h b/extensions/browser/api/socket/udp_socket.h
index bc655989882c..433c3a56943a 100644
--- a/extensions/browser/api/socket/udp_socket.h
+++ b/extensions/browser/api/socket/udp_socket.h
@@ -61,6 +61,9 @@ class UDPSocket : public Socket, public network::mojom::UDPSocketListener {
   // Sets broadcast to |enabled|. Can only be called after a successful Bind().
   void SetBroadcast(bool enabled, net::CompletionOnceCallback callback);

+  // Address reuse options must be set before Bind()/Connect() is called.
+  int SetAllowAddressReuse(bool allowed);
+
   const std::vector<std::string>& GetJoinedGroups() const;

  protected:
diff --git a/extensions/browser/api/sockets_udp/sockets_udp_api.cc b/extensions/browser/api/sockets_udp/sockets_udp_api.cc
index aedfe02ea34f..b8db6124bcd4 100644
--- a/extensions/browser/api/sockets_udp/sockets_udp_api.cc
+++ b/extensions/browser/api/sockets_udp/sockets_udp_api.cc
@@ -566,5 +566,30 @@ void SocketsUdpSetBroadcastFunction::OnCompleted(int net_result) {
   AsyncWorkCompleted();
 }

+SocketsUdpSetAllowAddressReuseFunction::
+    SocketsUdpSetAllowAddressReuseFunction() {}
+
+SocketsUdpSetAllowAddressReuseFunction::
+    ~SocketsUdpSetAllowAddressReuseFunction() {}
+
+bool SocketsUdpSetAllowAddressReuseFunction::Prepare() {
+  params_ = api::sockets_udp::SetAllowAddressReuse::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(params_.get());
+  return true;
+}
+
+void SocketsUdpSetAllowAddressReuseFunction::Work() {
+  ResumableUDPSocket* socket = GetUdpSocket(params_->socket_id);
+  if (!socket) {
+    error_ = kSocketNotFoundError;
+    return;
+  }
+
+  int net_result = socket->SetAllowAddressReuse(params_->allowed);
+  if (net_result != net::OK)
+    error_ = net::ErrorToString(net_result);
+  results_ = sockets_udp::SetAllowAddressReuse::Results::Create(net_result);
+}
+
 }  // namespace api
 }  // namespace extensions
diff --git a/extensions/browser/api/sockets_udp/sockets_udp_api.h b/extensions/browser/api/sockets_udp/sockets_udp_api.h
index ad9b06f28cde..a10eb22e358e 100644
--- a/extensions/browser/api/sockets_udp/sockets_udp_api.h
+++ b/extensions/browser/api/sockets_udp/sockets_udp_api.h
@@ -250,6 +250,25 @@ class SocketsUdpSetMulticastTimeToLiveFunction
   std::unique_ptr<sockets_udp::SetMulticastTimeToLive::Params> params_;
 };

+class SocketsUdpSetAllowAddressReuseFunction
+    : public UDPSocketAsyncApiFunction {
+ public:
+  DECLARE_EXTENSION_FUNCTION("sockets.udp.setAllowAddressReuse",
+                             SOCKETS_UDP_SETALLOWADDRESSREUSE)
+
+  SocketsUdpSetAllowAddressReuseFunction();
+
+ protected:
+  ~SocketsUdpSetAllowAddressReuseFunction() override;
+
+  // AsyncApiFunction
+  bool Prepare() override;
+  void Work() override;
+
+ private:
+  std::unique_ptr<sockets_udp::SetAllowAddressReuse::Params> params_;
+};
+
 class SocketsUdpSetMulticastLoopbackModeFunction
     : public UDPSocketAsyncApiFunction {
  public:
diff --git a/extensions/browser/extension_function_histogram_value.h b/extensions/browser/extension_function_histogram_value.h
index ffd2d40c50ab..1e64f88fcd61 100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -1500,6 +1500,7 @@ enum HistogramValue {
   PRINTING_CANCELJOB = 1437,
   WEBCAMPRIVATE_RESTORE_CAMERA_PRESET = 1449,
   WEBCAMPRIVATE_SET_CAMERA_PRESET = 1450,
+  SOCKETS_UDP_SETALLOWADDRESSREUSE = 1451,
   // Last entry: Add new entries above, then run:
   // python tools/metrics/histograms/update_extension_histograms.py
   ENUM_BOUNDARY = 1440
diff --git a/extensions/common/api/sockets_udp.idl b/extensions/common/api/sockets_udp.idl
index 784a31d25d77..696756dd5db2 100644
--- a/extensions/common/api/sockets_udp.idl
+++ b/extensions/common/api/sockets_udp.idl
@@ -124,6 +124,11 @@ namespace sockets.udp {
   // A negative value indicates an error.
   callback SetMulticastLoopbackModeCallback = void (long result);

+  // Callback from the <code>setAllowAddressReuse</code> method.
+  // |result| : The result code returned from the underlying network call.
+  // A negative value indicates an error.
+  callback SetAllowAddressReuseCallback = void (long result);
+
   // Callback from the <code>getJoinedGroupsCallback</code> method.
   // |groups| : Array of groups the socket joined.
   callback GetJoinedGroupsCallback = void (DOMString[] groups);
@@ -290,6 +295,15 @@ namespace sockets.udp {
         boolean enabled,
         SetMulticastLoopbackModeCallback callback);

+    // Sets whether the socket should allow address reuse.
+    // |socketId| : The socket ID.
+    // |allowed| : Indicate whether to allow or disallow address reuse.
+    // |callback| : Called when the configuration operation completes.
+    static void setAllowAddressReuse(
+        long socketId,
+        boolean allowed,
+        SetAllowAddressReuseCallback callback);
+
     // Gets the multicast group addresses the socket is currently joined to.
     // |socketId| : The socket ID.
     // |callback| : Called with an array of strings of the result.

This will also require running python tools/metrics/histograms/update_extension_histograms.py to update the tools/metrics/histograms/enums.xml file.

[RangerMauve]

Hey! Just wanted to follow up and see if there was anything else I could do for this.

[yrliou]

For udp_socket.cc/h and sockets_udp_api.cc/h, I could imagine to make it down to 1 line patch in udp_socket.h using chromium_src overwrite.
For histogram, extension APIs adding by brave for now are using UNKNOWN and does not patch the histogram.
I don't know what could be done for idl, probably need to patch it.

That being said, it seems like a good candidate to try to push to upstream so these patches could go away. The old patch seems to be almost got accepted by chromium but not due to fail on mac, which I suppose we don't have that issue anymore, right?

[RangerMauve]

Awesome. I don't have a mac handy right now but I'll ask around to see if anyone in my city is willing to let me use theirs.

I'll report after I've test it and see about cleaning up the patch from there. 😁

[RangerMauve]

Question: Is there a way for me to cross-compile to make a Mac build from my Linux machine?

I've got my hands on a Mac I could use but I don't think it has space. If not I'll try to look for other macs with more space on them. :P

[RangerMauve]

Trying to run npm run build -- --target_os=mac hopefully that'll work. 😅

[RangerMauve]

Gave up trying to source a laptop in meatspace. Gonna try compiling a binary on my machine and giving instructions to someone virtually. 😁

[RangerMauve]

I tried building a release version for mac, the src/out/mac_Release folder is showing up as 48 GB in size, and the src/out/mac_Release/brave binary is showing up as 6.6 GB. Is that correct? This seems a bit bigger than I expected. 😅

[RangerMauve]

Ah, just found the create_dist command. Might be a good to have that somewhere a bit more visible in the Wiki.

[RangerMauve]

Is cross compiling from linux even possible? It seems target_os=mac was wrong and I needed to use target_os=darwin. Not sure if that'll work, and it looks like this'll need another three hours of compilation. 😅

[RangerMauve]

Hey @yrliou I've made a branch of brave-core with my patches. https://github.com/RangerMauve/brave-core/tree/udp-fix

Would it be possible to trigger the regular brave CI to compile a DMG using them so I can send it out for tests?

[yrliou]

@RangerMauve a test PR is created to run CI, it failed because we just merged chromium 83 into master, please rebase on master and update your patch file if needed and push to the branch again, thanks.

[RangerMauve]

Will do, thank you! I've also got my hands on a macbook so I can test this out once the DMG is built. 😁

[RangerMauve]

@yrliou I just pushed the rebased version to my fork. Would you be able to update the branch for the CI when you have time? 😁

[yrliou]

@RangerMauve Failed with below, was your local build successful after rebasing? You might need to run npm run update_patches if your patch changes after chromium upgrade. Also, do we really need to patch histogram?

11:58:28  2 failed patches:
11:58:28  extensions/browser/extension_function_histogram_value.h
11:58:28    - Patch applied because: No corresponding .patchinfo file was found.
11:58:28    - Error - Program git exited with error code 1.
11:58:28  error: patch failed: extensions/browser/extension_function_histogram_value.h:1500
11:58:28  error: extensions/browser/extension_function_histogram_value.h: patch does not apply
11:58:28  
11:58:28  -------------------------------
11:58:28  tools/metrics/histograms/enums.xml
11:58:28    - Patch applied because: No corresponding .patchinfo file was found.
11:58:28    - Error - Program git exited with error code 1.
11:58:28  error: patch failed: tools/metrics/histograms/enums.xml:20682
11:58:28  error: tools/metrics/histograms/enums.xml: patch does not apply
11:58:28  
11:58:28  -------------------------------
11:58:28  Done applying patches.
11:58:28  Exiting as not all patches were successful!

[RangerMauve]

Ah dang. I'll check it out Thank you!

[RangerMauve]

Histagram was needed for adding the new JS method to the API. Though I could probably move the flag into the constructor instead?

[lidel]

IIUC the patch from 2017 used arg:

chrome.sockets.udp.create({ "allowAddressReuse": true }, onCreate)

@RangerMauve would doing something similar (instead of a new API method) work for us in the context of local discovery in Brave?

[RangerMauve]

@lidel Yeah, I was initially departing from that because of the way the multicast APIs were configuring the settings, but I think that adding it inside create is better. I'll make the change as part of figuring out this rebasing issue. 😁 It might make more sense for the node dgram API, too.

[RangerMauve]

K, reworked the API to have allowAddressReuse in the create() call, I tested it out and it all seems to be working. Force pushed a new set of patches.

[RangerMauve]

@yrliou Would you be able to get my latest changes into the CI when you have a chance? Thank you so much for your patience with all this BTW. 💜

[yrliou]

@RangerMauve np, pushed.

[RangerMauve]

Tested on Mac, getting the error ERR_ADDRESS_IN_USE. It seems the multicast-dns CLI is working in node, so it's likely a problem with the allow_address_reuse flag not being passed to whatever code is handling UDP sockets on Mac.

[RangerMauve]

Looking into where the flag is being used in the networking code.

It looks like on Linux it's using the SO_REUSEADDR option on the socket when SetReuseAddr is called on the underlying socket. The comments around there say that on MacOS X and iOS you need to use SO_REUSEPORT instead. I'll try adding a patch there to use that option on MacOS only and see if it helps.

[RangerMauve]

Gonna see how Node.js handles this case since they've got it working somehow.

[RangerMauve]

Here's how libuv handles it: https://github.com/nodejs/node/blob/3abb52fdb683c9c9ade1b2c7d16d0f640bbaacfd/deps/uv/src/unix/udp.c#L487

Here's how chromium handles it: https://chromium.googlesource.com/chromium/src/net/+/refs/heads/master/socket/socket_options.cc#33

Still doing some research to actually understand what's going on, but it seems I can do a check for #if defined(SO_REUSEPORT) and use it instead of SO_REUSEADDR 😂

[RangerMauve]

K, I've added another patch to my branch. It appears to be compiling fine on linux still. 😅

@yrliou would you mind building another dmg when you have a chance? 😁

[yrliou]

@RangerMauve np, pushed.

[RangerMauve]

Weeeird, it doesn't seem to have helped even though it's pretty much the same code as libuv. 🙃 Just pushed out another patch which uses the exact same variable as the libuv code, maybe that'll help. 🤷

[RangerMauve]

Hmm, actually, looking at the code for the multicast-dns module, it could be that we need to bind to a specific network interface for things to work.

https://github.com/mafintosh/multicast-dns/blob/master/index.js#L157

Gonna investigate why it's there tomorrow. No need to do another build in the meantime. 😁

[RangerMauve]

Seems I'll need to make use of the network interface API to get this working properly on Mac.

https://developer.chrome.com/apps/system_network

Are there any blockers for adding it to the safelist in the ipfs-companion extension?