Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739203003:
Measure how often DNS hostnames aren't in preferred name form. (Closed)
DescriptionMeasure how often DNS hostnames aren't in preferred name form.
As https://tools.ietf.org/html/rfc7719#section-2 explains, "host name" has a storied
history with many interpretations. Measure how often the hostnames passed to the
resolver aren't in the preferred name form [A-Za-z0-9-]+ (with support for _ as well),
to see where the DNS resolver code can deprecate and remove support for names
not in that form.
BUG=695474
Review-Url: https://codereview.chromium.org/2739203003
Cr-Commit-Position: refs/heads/master@{#459212}
Committed: https://chromium.googlesource.com/chromium/src/+/6170ab968eae5c5f683cdb88fa09e96b95f2b588
Patch Set 1 #
Total comments: 1
Patch Set 2 : We need to allow leading _. #
Total comments: 1
Patch Set 3 : Change the behavior to reporting via UMA; add documentation. #
Total comments: 8
Patch Set 4 : Rebase and respond to comments. #
Total comments: 1
Patch Set 5 : Record the UMA immediately after label validation in the for (;;) and before other checks. #
Total comments: 2
Patch Set 6 : Comment on when the histogram will be recorded. #Patch Set 7 : Rebase. #
Messages
Total messages: 51 (26 generated)
palmer@chromium.org changed reviewers: + rdsmith@chromium.org
The CQ bit was checked by palmer@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...
PTAL. This CL causes the bug to stop happening, allows IDN to still work (it's represented as Punycode at this layer, which passes the lexical tests), and comes with some tests we should have had all along. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
+juliatuttle, mgersh FTI. Glancing at the RFCs, it looks to me as if _ isn't permitted; just - & . (and . doesn't make sense in this context). Would you put in an RFC reference and some words of justification as to why this is more lenient than the RFC? (Or why it's in accord with the RFC and (implicitly) why I'm confused? :-}) Beyond that LGTM.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
I'm sorry to do this, but Not LGTM until we've file an Intent to Deprecate, per //net's policy to follow the Blink process. This is an ongoing topic of discussion across browsers, and we should not circumvent that without following our process for assessing the compatibility and ecosystem risk.
https://codereview.chromium.org/2739203003/diff/1/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/2739203003/diff/1/net/dns/dns_util.cc#newcode43 net/dns/dns_util.cc:43: } Newline between 42 & 43, plus a " //namespace" following.
> net/dns/dns_util.cc:43: } > Newline between 42 & 43, plus a " //namespace" following. Done. I'm going to fire up an Intent To Deprecate. We'll see how that goes. Also I found that we do need to support leading _, so I made that change. rdsmith: Well, views on the RFCs differ. :) I had always understood it to be that _ was technically forbidden, but in common practice it does happen, so from a pragmatic perspective we should allow it. But rsleevi says that basically any octet is potentially valid. The difference might be that the rules for A/AAAA records are different for SRV or other record types. I would like for us to support all and only what is really necessary on the open web.
The CQ bit was checked by palmer@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/03/10 21:10:37, palmer wrote: > > net/dns/dns_util.cc:43: } > > Newline between 42 & 43, plus a " //namespace" following. > > Done. > > I'm going to fire up an Intent To Deprecate. We'll see how that goes. Also I > found that we do need to support leading _, so I made that change. > > rdsmith: Well, views on the RFCs differ. :) I had always understood it to be > that _ was technically forbidden, but in common practice it does happen, so from > a pragmatic perspective we should allow it. But rsleevi says that basically any > octet is potentially valid. The difference might be that the rules for A/AAAA > records are different for SRV or other record types. I would like for us to > support all and only what is really necessary on the open web. Heh. My first project in Chrome was the cookie store--I'm very familiar with RFCs being not so much rules as ... guidelines :-}. I just wanted a reference and a comment as to what the thinking was for our deviations.
Oh yeah, I'll add some comments to that effect.
eroman@chromium.org changed reviewers: + eroman@chromium.org
https://codereview.chromium.org/2739203003/diff/20001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/2739203003/diff/20001/net/dns/dns_util.cc#new... net/dns/dns_util.cc:35: if (!isalnum(c) && c != '_') { Drive-by comment: isalnum() depends on the user's locale, which is not actually what we want here. In general the C string functions as full of traps and not usually appropriate for use in the //net protocol code.
OK, PTAL: I've changed the code to report validation failure via UMA instead of to fail hard, to not use isalnum, and have added documentation about where the policy comes from. Additionally, I had to move some code around, but ultimately I think |IsValidLabelCharacter| should go back to being a private implementation detail of |DNSDomainFromDot| (but still documented in that function's documentation, of course).
LGTM % updated commit description explaining what and why :) https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.cc#new... net/dns/dns_util.cc:73: UMA_HISTOGRAM_BOOLEAN("Net.ValidDNSName", valid_name); Do you want to record this after line 85/87? https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.h File net/dns/dns_util.h (right): https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.h#newc... net/dns/dns_util.h:33: // This function asserts a looser form of the restrictions in RFC 1123 (section https://tools.ietf.org/html/rfc7719#section-2 is meant to be the clarifying document about whose rules matter :) And include the link back to 1123 (under "Host name") https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.h#newc... net/dns/dns_util.h:43: NET_EXPORT_PRIVATE bool IsValidLabelCharacter(char c, bool is_first_char); pedantry: IsValidHostLabelCharacter? Since other labels can and do exceed the host record (implied A/AAAA, such as SRVNames) https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util_unitte... File net/dns/dns_util_unittest.cc (right): https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util_unitte... net/dns/dns_util_unittest.cc:17: } else { https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Discourages else after return
Description was changed from ========== Lexically validate DNS hostnames. BUG=695474 ========== to ========== Lexically validate DNS hostnames. Count valid/invalid hostnames with an UMA boolean, so that we can determine if we can deprecate and remove support for lexically-invalid hostnames. BUG=695474 ==========
palmer@chromium.org changed reviewers: + mpearson@chromium.org
https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.cc#new... net/dns/dns_util.cc:73: UMA_HISTOGRAM_BOOLEAN("Net.ValidDNSName", valid_name); > Do you want to record this after line 85/87? Oops, yes. Done. https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.h File net/dns/dns_util.h (right): https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.h#newc... net/dns/dns_util.h:33: // This function asserts a looser form of the restrictions in RFC 1123 (section > https://tools.ietf.org/html/rfc7719#section-2 is meant to be the clarifying document about whose rules matter :) And include the link back to 1123 (under "Host name") Done. https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util.h#newc... net/dns/dns_util.h:43: NET_EXPORT_PRIVATE bool IsValidLabelCharacter(char c, bool is_first_char); > pedantry: IsValidHostLabelCharacter? Since other labels can and do exceed the host record (implied A/AAAA, such as SRVNames) Done. https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util_unitte... File net/dns/dns_util_unittest.cc (right): https://codereview.chromium.org/2739203003/diff/40001/net/dns/dns_util_unitte... net/dns/dns_util_unittest.cc:17: } else { > Discourages else after return Done.
+mpearson for histograms.xml.
The CQ bit was checked by palmer@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...
https://codereview.chromium.org/2739203003/diff/60001/net/dns/dns_util.cc File net/dns/dns_util.cc (right): https://codereview.chromium.org/2739203003/diff/60001/net/dns/dns_util.cc#new... net/dns/dns_util.cc:83: UMA_HISTOGRAM_BOOLEAN("Net.ValidDNSName", valid_name); Did you mean to move this down further? That is, to after line 90 :) I just wasn't sure because of the short-circuits below
Description was changed from ========== Lexically validate DNS hostnames. Count valid/invalid hostnames with an UMA boolean, so that we can determine if we can deprecate and remove support for lexically-invalid hostnames. BUG=695474 ========== to ========== Measure how often DNS hostnames aren't in preferred name form As https://tools.ietf.org/html/rfc7719#section-2 explains, "host name" has a storied history with many interpretations. Measure how often the hostnames passed to the resolver aren't in the preferred name form [A-Za-z0-9-] (with support for _ as well), to see where the DNS resolver code can deprecate and remove support for names not in that form. BUG=695474 ==========
I made a stab at updating the description, but if you're unhappy with it, feel free to revert. My main goal was trying to capture the debate on whether "host name" always means "preferred name syntax" or something else, and that our goal is to enforce preferred name syntax (with a relaxation) rather than arbitrary octets.
Description was changed from ========== Measure how often DNS hostnames aren't in preferred name form As https://tools.ietf.org/html/rfc7719#section-2 explains, "host name" has a storied history with many interpretations. Measure how often the hostnames passed to the resolver aren't in the preferred name form [A-Za-z0-9-] (with support for _ as well), to see where the DNS resolver code can deprecate and remove support for names not in that form. BUG=695474 ========== to ========== Measure how often DNS hostnames aren't in preferred name form. As https://tools.ietf.org/html/rfc7719#section-2 explains, "host name" has a storied history with many interpretations. Measure how often the hostnames passed to the resolver aren't in the preferred name form [A-Za-z0-9-]+ (with support for _ as well), to see where the DNS resolver code can deprecate and remove support for names not in that form. BUG=695474 ==========
> Did you mean to move this down further? That is, to after line 90 :) > > I just wasn't sure because of the short-circuits below Actually, no; and I realize I didn't want to move it down before, either. I moved it back up to line 73. I want to record the measurement right after lexical validation and before any of the short-circuit "return false;"s after the for (;;) loop.
On 2017/03/22 at 01:15:17, rsleevi wrote: > I made a stab at updating the description, but if you're unhappy with it, feel free to revert. My main goal was trying to capture the debate on whether "host name" always means "preferred name syntax" or something else, and that our goal is to enforce preferred name syntax (with a relaxation) rather than arbitrary octets. Thanks; tweaked it slightly.
Still LGTM
histograms.xml lgtm modulo comment --mark https://codereview.chromium.org/2739203003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739203003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38023: + and remove support for arbitrary bytes in DNS names. Please state when this recorded. I.e., what things cause this function to be invoked. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
https://codereview.chromium.org/2739203003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739203003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:38023: + and remove support for arbitrary bytes in DNS names. > Please state when this recorded. I.e., what things cause this function to be invoked. Done.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, mpearson@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2739203003/#ps100001 (title: "Comment on when the histogram will be recorded.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for net/dns/dns_util.cc:
While running git apply --index -p1;
error: patch failed: net/dns/dns_util.cc:10
error: net/dns/dns_util.cc: patch does not apply
Patch: net/dns/dns_util.cc
Index: net/dns/dns_util.cc
diff --git a/net/dns/dns_util.cc b/net/dns/dns_util.cc
index
f80eaee9c54a80fbd6a96ea13dc4d78167c629ad..9d21737f0758d121a9e896de776da6b4b5a8255f
100644
--- a/net/dns/dns_util.cc
+++ b/net/dns/dns_util.cc
@@ -10,22 +10,38 @@
#include <cstring>
#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_split.h"
#include "build/build_config.h"
#include "net/base/address_list.h"
+#if defined(OS_POSIX)
+#include <netinet/in.h>
+#if !defined(OS_NACL)
+#include <net/if.h>
+#if !defined(OS_ANDROID)
+#include <ifaddrs.h>
+#endif // !defined(OS_ANDROID)
+#endif // !defined(OS_NACL)
+#endif // defined(OS_POSIX)
+
+#if defined(OS_ANDROID)
+#include "net/android/network_library.h"
+#endif
+
namespace net {
// Based on DJB's public domain code.
bool DNSDomainFromDot(const base::StringPiece& dotted, std::string* out) {
const char* buf = dotted.data();
- unsigned n = dotted.size();
+ size_t n = dotted.size();
char label[63];
size_t labellen = 0; /* <= sizeof label */
char name[255];
size_t namelen = 0; /* <= sizeof name */
char ch;
+ bool valid_name = true;
for (;;) {
if (!n)
@@ -46,9 +62,16 @@ bool DNSDomainFromDot(const base::StringPiece& dotted,
std::string* out) {
}
if (labellen >= sizeof label)
return false;
+ if (!IsValidHostLabelCharacter(ch, labellen == 0)) {
+ // TODO(palmer): In the future, when we can remove support for invalid
+ // names, return false here instead (and remove the UMA counter).
+ valid_name = false;
+ }
label[labellen++] = ch;
}
+ UMA_HISTOGRAM_BOOLEAN("Net.ValidDNSName", valid_name);
+
// Allow empty label at end of name to disable suffix search.
if (labellen) {
if (namelen + labellen + 1 > sizeof name)
@@ -74,6 +97,11 @@ bool IsValidDNSDomain(const base::StringPiece& dotted) {
return DNSDomainFromDot(dotted, &dns_formatted);
}
+bool IsValidHostLabelCharacter(char c, bool is_first_char) {
+ return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') ||
+ (c >= '0' && c <= '9') || (!is_first_char && c == '-') || c == '_';
+}
+
std::string DNSDomainToString(const base::StringPiece& domain) {
std::string ret;
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, rdsmith@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2739203003/#ps120001 (title: "Rebase.")
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": 120001, "attempt_start_ts": 1490298290046270,
"parent_rev": "b4dde882f6c2b2be84035b53f60ec286d347bc50", "commit_rev":
"6170ab968eae5c5f683cdb88fa09e96b95f2b588"}
Message was sent while issue was closed.
Description was changed from ========== Measure how often DNS hostnames aren't in preferred name form. As https://tools.ietf.org/html/rfc7719#section-2 explains, "host name" has a storied history with many interpretations. Measure how often the hostnames passed to the resolver aren't in the preferred name form [A-Za-z0-9-]+ (with support for _ as well), to see where the DNS resolver code can deprecate and remove support for names not in that form. BUG=695474 ========== to ========== Measure how often DNS hostnames aren't in preferred name form. As https://tools.ietf.org/html/rfc7719#section-2 explains, "host name" has a storied history with many interpretations. Measure how often the hostnames passed to the resolver aren't in the preferred name form [A-Za-z0-9-]+ (with support for _ as well), to see where the DNS resolver code can deprecate and remove support for names not in that form. BUG=695474 Review-Url: https://codereview.chromium.org/2739203003 Cr-Commit-Position: refs/heads/master@{#459212} Committed: https://chromium.googlesource.com/chromium/src/+/6170ab968eae5c5f683cdb88fa09... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/6170ab968eae5c5f683cdb88fa09... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
