Bug 134237 - [Win] MSVC mishandles enums in bitfields - breaks Cross-Origin Access Control
Summary: [Win] MSVC mishandles enums in bitfields - breaks Cross-Origin Access Control
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 126972
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-23 21:31 PDT by Brent Fulgham
Modified: 2014-06-24 12:49 PDT (History)
18 users (show)

See Also:


Attachments
Patch (2.01 KB, patch)
2014-06-23 21:48 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.85 KB, patch)
2014-06-24 10:01 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (38.26 KB, patch)
2014-06-24 11:17 PDT, Brent Fulgham
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-06-23 21:31:18 PDT
Consider this code in CrossOriginAccessControl.cpp (passesAccessControlCheck):

    if (accessControlOriginString == "*" && includeCredentials == DoNotAllowStoredCredentials)

Amazingly, although includeCredentials is set to 'DoNotAllowStoredCredentials', this test failed:

> p includeCredentials
DoNotAllowStoredCredentials | -2 (-1)
> p DoNotAllowStoredCredentials
DoNotAllowStoredCredentials (1)
> DoNotAllowStoredCredentials == includeCredentials
false

This change was introduced in http://trac.webkit.org/changeset/161958/trunk/Source/WebCore/loader/ResourceLoaderOptions.h.

MSVC does strange things with bit fields containing enumerations. This has been documented in a few places:

http://connect.microsoft.com/VisualStudio/feedback/details/828892/vc-2013-miscompilation-with-enums-and-bit-fields
http://objectmix.com/c/749570-enum-bitfield-visual-studio-bug-not-3.html

Small test case:

  #include <iostream>

  enum E {
    A = 0, B, C, D
  };

  struct S {
    E e : 2;
  };

  int main()
  {
      S s;
      s.e = D;
      std::cout << s.e << ' ' << (s.e == D) << std::endl;
  }

Output on MSVC: -1 0
Output on G++/clang++ (correct according to the C++ standard): 3 1

Microsoft appears to think this behavior is within specification, since their enumerated type is based on int, not unsigned. They use a sign bit, which means our one-bit enum fields only ever hold the sign bit resulting in weird behavior.
Comment 1 Radar WebKit Bug Importer 2014-06-23 21:40:02 PDT
<rdar://problem/17430588>
Comment 2 Brent Fulgham 2014-06-23 21:48:44 PDT
Created attachment 233680 [details]
Patch
Comment 3 Anders Carlsson 2014-06-24 07:11:27 PDT
Comment on attachment 233680 [details]
Patch

I think what we usually do in cases like this is to just use unsigned integer types, let's do that instead.
Comment 4 Brent Fulgham 2014-06-24 10:01:05 PDT
Created attachment 233714 [details]
Patch
Comment 5 Brent Fulgham 2014-06-24 10:01:43 PDT
I found a few other cases where this was happening and have expanded the bug to cover them.
Comment 6 Brent Fulgham 2014-06-24 11:17:41 PDT
Created attachment 233720 [details]
Patch
Comment 7 Brent Fulgham 2014-06-24 11:20:24 PDT
See also Bug 134252.
Comment 8 Michael Saboff 2014-06-24 11:42:43 PDT
Comment on attachment 233720 [details]
Patch

r=me.  Fix Mac build.
Comment 9 Brent Fulgham 2014-06-24 12:49:53 PDT
Committed r170381: <http://trac.webkit.org/changeset/170381>