Bug 238797

Summary: [GTK][WPE] RemoteInspector add support for IPv6
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: 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 Flags
Patch
none
Patch aperez: review+

Description Carlos Garcia Campos 2022-04-05 03:19:27 PDT
Make it possible to connect to remote inspector server using IPv6 address.
Comment 1 Carlos Garcia Campos 2022-04-05 03:24:13 PDT
Created attachment 456686 [details]
Patch
Comment 2 Adrian Perez 2022-04-05 04:56:59 PDT
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 3 Carlos Garcia Campos 2022-04-05 05:08:05 PDT
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.
Comment 4 Carlos Garcia Campos 2022-04-05 05:09:19 PDT
(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 5 Adrian Perez 2022-04-05 05:43:49 PDT
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
Comment 6 Carlos Garcia Campos 2022-04-05 06:47:19 PDT
(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 7 Michael Catanzaro 2022-04-05 08:11:02 PDT
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 8 Carlos Garcia Campos 2022-04-06 05:46:28 PDT
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'
Comment 9 Carlos Garcia Campos 2022-04-06 06:05:31 PDT
Created attachment 456811 [details]
Patch
Comment 10 Michael Catanzaro 2022-04-06 14:33:09 PDT
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).
Comment 11 Michael Catanzaro 2022-04-06 14:34:59 PDT
(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.
Comment 12 Carlos Garcia Campos 2022-04-07 03:14:10 PDT
(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.
Comment 13 Carlos Garcia Campos 2022-04-07 03:15:47 PDT
Committed r292527 (249365@trunk): <https://commits.webkit.org/249365@trunk>
Comment 14 Michael Catanzaro 2022-04-07 07:24:13 PDT
(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