Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968573002:
Check the return value of base::StringToInt() in (Closed)
DescriptionCheck the return value of base::StringToInt() in
ProxyConfigServiceLinux.
The code was assuming it would set the result to 0 on failure, however
this is not part of the API contract (the actual contract is weirder).
This wouldn't have caused any real bugs, other than some
different interpretations of invalid KDE proxy configurations.
The CL changes some of those edge cases in arbitrary ways (i.e. overflowed integer is now 0 rather than max_int).
BUG=596573
Review-Url: https://codereview.chromium.org/2968573002
Cr-Commit-Position: refs/heads/master@{#483553}
Committed: https://chromium.googlesource.com/chromium/src/+/e44498c3ea4925a4868cad26fcee8fb91cd139cd
Patch Set 1 #
Total comments: 8
Patch Set 2 : address mmenke's comments #
Messages
Total messages: 19 (13 generated)
Description was changed from ========== Check the return value of base::StringToInt() in ProxyConfigServiceLinux. The code was assuming it would set the result to 0 on failure, however this is not part of the API contract (the actual contract is much This probably wouldn't have caused any real bugs, other than some different interpretations of invalid proxy configurations. BUG=596573 ========== to ========== Check the return value of base::StringToInt() in ProxyConfigServiceLinux. The code was assuming it would set the result to 0 on failure, however this is not part of the API contract (the actual contract is much weirder). This probably wouldn't have caused any real bugs, other than some different interpretations of invalid proxy configurations. BUG=596573 ==========
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Check the return value of base::StringToInt() in ProxyConfigServiceLinux. The code was assuming it would set the result to 0 on failure, however this is not part of the API contract (the actual contract is much weirder). This probably wouldn't have caused any real bugs, other than some different interpretations of invalid proxy configurations. BUG=596573 ========== to ========== Check the return value of base::StringToInt() in ProxyConfigServiceLinux. The code was assuming it would set the result to 0 on failure, however this is not part of the API contract (the actual contract is weirder). This wouldn't have caused any real bugs, other than some different interpretations of invalid KDE proxy configurations. The CL changes some of those edge cases in arbitrary ways (i.e. overflowed integer is now 0 rather than max_int). BUG=596573 ==========
eroman@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_linux.cc (right): https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux.cc:1116: default: // No proxy, or maybe kioslaverc syntax error. (Previously didn't account for negative numbers) https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux.cc:1137: reversed_bypass_list_ = (value == "true" || int_value); (Some of this parsing is weird... but I am just trying to address the specific use of StringToInt() in this CL, so meh). https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_linux_unittest.cc (right): https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux_unittest.cc:1369: TEST_DESC("Correctly parse bypass list with ReversedException=false"), This is weird -- it tries parsing "false" as a number, and fails, so treats it as 0 (false). Hmm. https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux_unittest.cc:1388: "ReversedException=18446744073709551617"), Previously this would be interpreted as true (because overflow causes non-zero result) whereas now it is false. Which is equally strange, but not something I think matters one way or another.
LGTM https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_linux.cc (right): https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux.cc:1137: reversed_bypass_list_ = (value == "true" || int_value); On 2017/06/29 21:11:18, eroman wrote: > (Some of this parsing is weird... but I am just trying to address the specific > use of StringToInt() in this CL, so meh). Could you are least change this to: reversed_bypass_list_ = (value == "true" || StringToIntOrDefault(value, 0) != 0); "bool || int" is just weird, and brings back traumatizing memories of writing Javascript. https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_linux_unittest.cc (right): https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux_unittest.cc:1163: TEST_DESC("No proxying (negative value)"), Maybe a test in each group where the ProxyType is not a number. "ProxyType=AB-" and "ReversedException=noitpecxE" are my suggestions. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_linux.cc (right): https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux.cc:1137: reversed_bypass_list_ = (value == "true" || int_value); On 2017/06/29 22:21:20, mmenke wrote: > On 2017/06/29 21:11:18, eroman wrote: > > (Some of this parsing is weird... but I am just trying to address the specific > > use of StringToInt() in this CL, so meh). > > Could you are least change this to: > > reversed_bypass_list_ = (value == "true" || StringToIntOrDefault(value, 0) != > 0); > > "bool || int" is just weird, and brings back traumatizing memories of writing > Javascript. Done. https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... File net/proxy/proxy_config_service_linux_unittest.cc (right): https://codereview.chromium.org/2968573002/diff/1/net/proxy/proxy_config_serv... net/proxy/proxy_config_service_linux_unittest.cc:1163: TEST_DESC("No proxying (negative value)"), On 2017/06/29 22:21:20, mmenke wrote: > Maybe a test in each group where the ProxyType is not a number. "ProxyType=AB-" > and "ReversedException=noitpecxE" are my suggestions. :) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still LGTM
The CQ bit was unchecked by eroman@chromium.org
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498779215693570,
"parent_rev": "8f2493210c75d3ebe2ddabf42eb7b22cf8cecee4", "commit_rev":
"e44498c3ea4925a4868cad26fcee8fb91cd139cd"}
Message was sent while issue was closed.
Description was changed from ========== Check the return value of base::StringToInt() in ProxyConfigServiceLinux. The code was assuming it would set the result to 0 on failure, however this is not part of the API contract (the actual contract is weirder). This wouldn't have caused any real bugs, other than some different interpretations of invalid KDE proxy configurations. The CL changes some of those edge cases in arbitrary ways (i.e. overflowed integer is now 0 rather than max_int). BUG=596573 ========== to ========== Check the return value of base::StringToInt() in ProxyConfigServiceLinux. The code was assuming it would set the result to 0 on failure, however this is not part of the API contract (the actual contract is weirder). This wouldn't have caused any real bugs, other than some different interpretations of invalid KDE proxy configurations. The CL changes some of those edge cases in arbitrary ways (i.e. overflowed integer is now 0 rather than max_int). BUG=596573 Review-Url: https://codereview.chromium.org/2968573002 Cr-Commit-Position: refs/heads/master@{#483553} Committed: https://chromium.googlesource.com/chromium/src/+/e44498c3ea4925a4868cad26fcee... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e44498c3ea4925a4868cad26fcee... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
