| Summary: | [GTK][WPE] RemoteInspector add support for IPv6 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aperez, bugs-noreply, mcatanzaro | ||||||
| Priority: | P2 | Keywords: | Gtk | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Carlos Garcia Campos
2022-04-05 03:19:27 PDT
Created attachment 456686 [details]
Patch
Comment on attachment 456686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456686&action=review > Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:83 > + inspectorServerAddress.reset(g_strdup_printf("%s:%u", host.get(), inspectorPort)); Is there any reason why cannot use g_inet_address_to_string() always here instead of checking the address family and performing the conversion by hand? A quick test shows that it should work as inteded: >>> from gi.repository import Gio >>> Gio.InetSocketAddress.new_from_string("::0", 8000).to_string() '[::]:8000' >>> Gio.InetSocketAddress.new_from_string("0.0.0.0", 8000).to_string() '0.0.0.0:8000' >>> Comment on attachment 456686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456686&action=review >> Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:83 >> + inspectorServerAddress.reset(g_strdup_printf("%s:%u", host.get(), inspectorPort)); > > Is there any reason why cannot use g_inet_address_to_string() always here > instead of checking the address family and performing the conversion by > hand? A quick test shows that it should work as inteded: Yes, g_inet_address_to_string() always returns the address without the square brackets. We could create a new GSocketInetAddress with the required port and then call to_string, but I just avoided to create another GObject. (In reply to Adrian Perez from comment #2) > Comment on attachment 456686 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=456686&action=review > > > Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:83 > > + inspectorServerAddress.reset(g_strdup_printf("%s:%u", host.get(), inspectorPort)); > > Is there any reason why cannot use g_inet_address_to_string() always here > instead of checking the address family and performing the conversion by > hand? A quick test shows that it should work as inteded: > > >>> from gi.repository import Gio > >>> Gio.InetSocketAddress.new_from_string("::0", 8000).to_string() > '[::]:8000' > >>> Gio.InetSocketAddress.new_from_string("0.0.0.0", 8000).to_string() > '0.0.0.0:8000' > >>> That's InetSocketAddress, not InetAddress. That gio APIS are quite confusing wityh so many address classes Comment on attachment 456686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456686&action=review >>>> Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:83 >>>> + inspectorServerAddress.reset(g_strdup_printf("%s:%u", host.get(), inspectorPort)); >>> >>> Is there any reason why cannot use g_inet_address_to_string() always here >>> instead of checking the address family and performing the conversion by >>> hand? A quick test shows that it should work as inteded: >> >> Yes, g_inet_address_to_string() always returns the address without the square brackets. We could create a new GSocketInetAddress with the required port and then call to_string, but I just avoided to create another GObject. > > That's InetSocketAddress, not InetAddress. That gio APIS are quite confusing wityh so many address classes But here we *do* have a GInetSocketAddress, note how in line 77 above there is an explicit checked cast using the G_INET_SOCKET_ADDRESS() macro. Note that the _to_string() method is virtual, and overriden by GInetSocketAddress: when using g_inet_address_to_string() on a GInetSocketAddress the overriden method from GInetSocketAddress will be used. The above can just be: GUniquePtr<char> inspectorServerAddress(g_inet_address_to_string(G_INET_ADDRESS(socketAddress.get())); Hence, if one calls g_inet_address_to_string() the implementa (In reply to Adrian Perez from comment #5) > Comment on attachment 456686 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=456686&action=review > > >>>> Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:83 > >>>> + inspectorServerAddress.reset(g_strdup_printf("%s:%u", host.get(), inspectorPort)); > >>> > >>> Is there any reason why cannot use g_inet_address_to_string() always here > >>> instead of checking the address family and performing the conversion by > >>> hand? A quick test shows that it should work as inteded: > >> > >> Yes, g_inet_address_to_string() always returns the address without the square brackets. We could create a new GSocketInetAddress with the required port and then call to_string, but I just avoided to create another GObject. > > > > That's InetSocketAddress, not InetAddress. That gio APIS are quite confusing wityh so many address classes > > But here we *do* have a GInetSocketAddress, note how in line 77 above there > is an explicit checked cast using the G_INET_SOCKET_ADDRESS() macro. Note > that the _to_string() method is virtual, and overriden by GInetSocketAddress: > when using g_inet_address_to_string() on a GInetSocketAddress the overriden > method from GInetSocketAddress will be used. > > The above can just be: > > GUniquePtr<char> > inspectorServerAddress(g_inet_address_to_string(G_INET_ADDRESS(socketAddress. > get())); But that's not the port we want, we would need to create a new GInetSocketAddress with the same host but the new port and then call to_string(). > Hence, > if one calls g_inet_address_to_string() the implementa Comment on attachment 456686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456686&action=review > Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:194 > - g_warning("Failed to start remote inspector server on %s:%u: %s\n", address, port, error->message); > + GUniquePtr<char> address(g_socket_connectable_to_string(G_SOCKET_CONNECTABLE(socketAddress.get()))); > + g_warning("Failed to start remote inspector server on %s: %s\n", address.get(), error->message); No trailing \n! (Does other preexisting code have this problem?) > Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp:71 > + char* addressPtr = inspectorAddress.get(); > + if (addressPtr[0] == '[' && *(portPtr - 2) == ']') { > + // Strip the square brackets. > + addressPtr++; > + *(portPtr - 2) = '\0'; > + } Careful: if for some reason the colon is the first or second character in the string, then you'll have memory corruption here. Got to make sure it's safe before dereferencing *(portPtr - 2) Comment on attachment 456686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456686&action=review >> Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorServer.cpp:194 >> + g_warning("Failed to start remote inspector server on %s: %s\n", address.get(), error->message); > > No trailing \n! (Does other preexisting code have this problem?) I think I fixed the others in previous patches >> Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp:71 >> + } > > Careful: if for some reason the colon is the first or second character in the string, then you'll have memory corruption here. Got to make sure it's safe before dereferencing *(portPtr - 2) This case is actually covered already. char* portPtr = g_strrstr(inspectorAddress.get(), ":"); if portPtr is nullptr we return early, so we know there's ':' *portPtr = '\0'; portPtr++; we change ':' by '\0' and move to the next character auto port = g_ascii_strtoull(portPtr, nullptr, 10); if portPtr at this point points to '\0' strtoull fails, so we return early so after this portPtr - 2 is always the position before the ':' that is now '\0' Created attachment 456811 [details]
Patch
Comment on attachment 456686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456686&action=review >>> Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp:71 >>> + } >> >> Careful: if for some reason the colon is the first or second character in the string, then you'll have memory corruption here. Got to make sure it's safe before dereferencing *(portPtr - 2) > > This case is actually covered already. > > char* portPtr = g_strrstr(inspectorAddress.get(), ":"); > if portPtr is nullptr we return early, so we know there's ':' > > *portPtr = '\0'; > portPtr++; > we change ':' by '\0' and move to the next character > > auto port = g_ascii_strtoull(portPtr, nullptr, 10); > if portPtr at this point points to '\0' strtoull fails, so we return early > > so after this portPtr - 2 is always the position before the ':' that is now '\0' Say the input string is ":1080". You replace the ':' with NUL '\0' and advance portPtr to point to the 1 in 1080. Next, you check *(portPtr -2) == ']'. Here, portPtr -1 is the '\0' and portPtr - 2 is a buffer underflow. Am I doing something wrong? You're saying (portPtr - 2) is the '\0', but it looks like that's actually (portPtr - 1). (In reply to Michael Catanzaro from comment #10) > You're saying (portPtr - 2) is the '\0', but it looks like that's actually (portPtr - 1). Sorry, I misread your comment. OK, we agree that (portPtr - 2) is one position before the '\0' that was originally ':'. Problem is, if the input string is bogus and nothing is before the ':', then that is out of bounds. (In reply to Michael Catanzaro from comment #11) > (In reply to Michael Catanzaro from comment #10) > > You're saying (portPtr - 2) is the '\0', but it looks like that's actually (portPtr - 1). > > Sorry, I misread your comment. OK, we agree that (portPtr - 2) is one > position before the '\0' that was originally ':'. Problem is, if the input > string is bogus and nothing is before the ':', then that is out of bounds. In that case we don't reach portPtr - 2 because we first check that addressPtr[0] == '[' and it's ':' in that case. Committed r292527 (249365@trunk): <https://commits.webkit.org/249365@trunk> (In reply to Carlos Garcia Campos from comment #12) > In that case we don't reach portPtr - 2 because we first check that > addressPtr[0] == '[' and it's ':' in that case. Ah, yes :P |