Bug 185555 – -Wmemset-elt-size warning in LibWebRTCSocket constructor

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

GCC is getting smarter. This looks bad:

../../Source/WebKit/WebProcess/Network/webrtc/LibWebRTCSocket.cpp:57:44: warning: ‘memset’ used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
     memset(&m_options, 1, MAX_SOCKET_OPTION);
                                            ^

I assume the intent of the code was to set all of m_options to 1, but it's only setting the first quarter of m_options (assuming the usual sizeof(int) == 4), right?

[Michael Catanzaro]

Created attachment 340210 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 340210 [details]
Patch

Clearing flags on attachment: 340210

Committed r231755: <https://trac.webkit.org/changeset/231755>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40217250>

[Darin Adler]

Comment on attachment 340210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340210&action=review

> Source/WebKit/WebProcess/Network/webrtc/LibWebRTCSocket.cpp:57
> -    memset(&m_options, 1, MAX_SOCKET_OPTION);
> +    memset(&m_options, 1, MAX_SOCKET_OPTION * sizeof(m_options[0]));

This code is extremely peculiar and I don’t think our change is good. Why do we want to set each byte of all the integers in the m_options array to 1? That means the integers will be set to values like 0x01010101 = 16843009. If we want to set the options integers to 1 we need to use std::fill_n instead of std::memset.

[Darin Adler]

I suggest:

    std::fill_n(&m_options[0], MAX_SOCKET_OPTION, 1);

[Michael Catanzaro]

Reverted r231755 for reason:

Change is not correct

Committed r231781: <https://trac.webkit.org/changeset/231781>

[Michael Catanzaro]

You're right, my patch was nonsense. Not good.

I'm not sure if your suggestion is right either, though. I looked around to figure out what m_options is supposed to be, and found this in Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/socket.h:

  enum Option {
    OPT_DONTFRAGMENT,
    OPT_RCVBUF,      // receive buffer size
    OPT_SNDBUF,      // send buffer size
    OPT_NODELAY,     // whether Nagle algorithm is enabled
    OPT_IPV6_V6ONLY, // Whether the socket is IPv6 only.
    OPT_DSCP,        // DSCP code
    OPT_RTP_SENDTIME_EXTN_ID,  // This is a non-traditional socket option param.
                               // This is specific to libjingle and will be used
                               // if SendTime option is needed at socket level.
  };
  virtual int GetOption(Option opt, int* value) = 0;
  virtual int SetOption(Option opt, int value) = 0;

m_options is indexed by Option, i.e. the first element of m_options is OPT_DONTFRAGMENT, the second element is OPT_RCVBUF, etc. So there are seven options/elements total in m_options, and each can be set to arbitrary integer values. So if we set each array element to 1, that would be equivalent to setting the value of all those options to 1 (DONTFRAGMENT = 1, receive buffer size = 1, send buffer size = 1, Nagle algorithm enabled = 1, IPv6 only = 1...). That seems like odd default behavior for a newly-constructed LibWebRTCSocket, so I'll guess that's probably not right.

Youenn, any idea how it's supposed to work?

P.S. The declaration of MAX_SOCKET_OPTION in LibWebRTCSocket.h will break in the future the next time a new Option is added, which is not great either.

[Darin Adler]

Looks like the GetOption function treats -1 as a special value meaning not set. So maybe the intent was to initialize to -1?

If so, then if we want these to be optional values that might not be set, std::optional is a clearer way to do it rather than using a magic value of -1. If we use std::optional we don’t need to use std::fill or std::memset since the constructor will take care of everything.

But I’m sure Youenn has a better understanding of what we’re really after.

[youenn fablet]

(In reply to Darin Adler from comment #9)
> Looks like the GetOption function treats -1 as a special value meaning not
> set. So maybe the intent was to initialize to -1?
> 
> If so, then if we want these to be optional values that might not be set,
> std::optional is a clearer way to do it rather than using a magic value of
> -1. If we use std::optional we don’t need to use std::fill or std::memset
> since the constructor will take care of everything.
> 
> But I’m sure Youenn has a better understanding of what we’re really after.

GetOption is not used in practice, SetOption is the one that is used.
GetOption is indeed currently very broken :(
We should probably do something like:
- Use std::optional<> as suggested by Darin
- If the std::optional<> value is not set, return -1. Ideally, we should be getting the value from NetworkProcess but I do not think this is worth it.

Michael, I can go ahead if that is ok with you.

[Michael Catanzaro]

Sounds good.

[youenn fablet]

Created attachment 340651 [details]
Patch

[Michael Catanzaro]

Comment on attachment 340651 [details]
Patch

Wouldn't it be better to remove GetOption instead of returning potentially-stale values...?

[youenn fablet]

(In reply to Michael Catanzaro from comment #13)
> Comment on attachment 340651 [details]
> Patch
> 
> Wouldn't it be better to remove GetOption instead of returning
> potentially-stale values...?

We need to implement it to instantiate LibWebRTCSocket.
GetOption is used in some unit tests, maybe logging as well.
I doubt this could be upstreamed very easily.

We could make the implementation even simpler by always returning -1 but this would increase breakage risk if some libwebrtc code starts to use GetOption for real.

We could make it smarter, given the number of actually used options is pretty limited
Mostly OPT_RCVBUF and OPT_SNDBUF are useful.
OPT_NODELAY should be set for all TCP sockets.

Or we could make it fully functional by doing IPC to the NetworkProcess.
Given all these calls should be done in a background thread, we would not need to resort on sync IPC. This seems a bit like overkill to me.

[youenn fablet]

FWIW, Chrome implementation is close to this lazy implementation.
The main differences:
- It has a static list of the options that Chrome supports. GetOption return -1 if not supported
- If the value is not previously set, GetOption return 0 and the out value parameter is set to -1

[EWS Watchlist]

Comment on attachment 340651 [details]
Patch

Attachment 340651 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7717459

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 340671 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 340651 [details]
Patch

Clearing flags on attachment: 340651

Committed r231956: <https://trac.webkit.org/changeset/231956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.