Return value of ReadSupportsQuic() is not being checked by HttpServerPropertiesManager |
||
Issue descriptionReadSupportsQuic() is called here: https://chromium.googlesource.com/chromium/src/+/c93ce0dff34a64ff092676ce0ab731052bae0cab/net/http/http_server_properties_manager.cc#504 One of its output parameters is an IPAddress, which may not get assigned, however it is subsequently used unconditionally: https://chromium.googlesource.com/chromium/src/+/c93ce0dff34a64ff092676ce0ab731052bae0cab/net/http/http_server_properties_manager.cc#592 Is this flow correct? Is the assumption that the IPAddress will be !IsValid() or empty()? Glancing at the first few functions involved, there is no mention of such contracts.
,
Mar 29 2018
Thanks for taking a look Ryan! I was mainly looking at this for https://chromium-review.googlesource.com/c/chromium/src/+/985907, for which I am confident the translation is equivalent to before (resets to empty IPAddress, which is what the default initialized value was too). If the rest of the code that sets and consumes the empty IPAddress looks good to you, feel free to close this issue.
,
Mar 30 2018
I think it looks good to me. Thanks for checking! |
||
►
Sign in to add a comment |
||
Comment 1 by rch@chromium.org
, Mar 29 2018As I read the code, it looks like the resulting IP is used here: bool HttpServerPropertiesImpl::GetSupportsQuic(IPAddress* last_address) const { if (last_quic_address_.empty()) return false; *last_address = last_quic_address_; return true; } Which seems to to do the needful if it's empty(). That being said, the place where ReadSupportsQuic is called, it appears to have a correctly initialized IPAddress, right? It just doesn't get set to a new value if Read fails. If so, I think this is ok...