APIClient::initialize does not build with newer gcc

• Component: WebKit2
• Status: RESOLVED INVALID

[Balazs Kelemen]

With -Werror and -Wextra - which is the default configuration for most of the ports - and gcc 4.5.1 this is the build error:
cc1plus: warnings being treated as errors
/home/balazs/work/master_clean/Source/WebKit2/Shared/APIClient.h: In member function 'void WebKit::WebGeolocationManagerProxy::initializeProvider(const WKGeolocationProvider*)':
/home/balazs/work/master_clean/Source/WebKit2/Shared/APIClient.h:52:13: error: array subscript is below array bounds

The guilty code part is:
if (client && client->version < currentVersion)
    memcpy(&m_client, client, APIClientTraits<ClientInterface>::interfaceSizesByVersion[client->version]);

Both currentVersion and client->version are int's and currentVersion - which is a template parameter - is 0.
So the compiler assumes that the memcpy line indexes the array with a negative number.
Seems like it's a bit tricky to write warning free template code.

[Balazs Kelemen]

Created attachment 107839 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 107839 [details]
Patch

It looks like this restructuring will give bad behavior in a non-debug build if the client passed in is of a newer version than is expected. Previously that would result in m_client being initialized to zero. Now it’ll attempt to memcpy the size of the version passed in, which will be undefined… That seems bad?

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 107839 [details])
> It looks like this restructuring will give bad behavior in a non-debug build if the client passed in is of a newer version than is expected. Previously that would result in m_client being initialized to zero. Now it’ll attempt to memcpy the size of the version passed in, which will be undefined… That seems bad?

Yep, you are right, I didn't think that we should handle that case. It seems like there is no other possible solution for this than turning client->version from int to unsigned - but I guess it would be an API breakage so it's not an option. However it turned out that it is only a problem with gcc-4.5.1. 4.5.2 does not warn about that. Furthermore I couldn't reproduce the issue with a small example so maybe my way of though is false and this is just a compiler bug.