Bug 68350 - APIClient::initialize does not build with newer gcc
Summary: APIClient::initialize does not build with newer gcc
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 05:40 PDT by Balazs Kelemen
Modified: 2011-09-20 01:02 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.77 KB, patch)
2011-09-19 05:48 PDT, Balazs Kelemen
mrowe: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-09-19 05:40:52 PDT
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.
Comment 1 Balazs Kelemen 2011-09-19 05:48:14 PDT
Created attachment 107839 [details]
Patch
Comment 2 Mark Rowe (bdash) 2011-09-19 10:40:44 PDT
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?
Comment 3 Balazs Kelemen 2011-09-20 01:02:14 PDT
(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.