Bug 238797 – [GTK][WPE] RemoteInspector add support for IPv6

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Make it possible to connect to remote inspector server using IPv6 address.

[Carlos Garcia Campos]

Created attachment 456686 [details]
Patch

[Adrian Perez]

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'
>>>

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

(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

[Adrian Perez]

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

[Carlos Garcia Campos]

(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

[Michael Catanzaro]

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)

[Carlos Garcia Campos]

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'

[Carlos Garcia Campos]

Created attachment 456811 [details]
Patch

[Michael Catanzaro]

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).

[Michael Catanzaro]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r292527 (249365@trunk): <https://commits.webkit.org/249365@trunk>

[Michael Catanzaro]

(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