Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2539583002:
Use overflow-safe string-to-int parsing methods for FTP ports. (Closed)
DescriptionUse overflow-safe string-to-int parsing methods for FTP ports.
BUG=667779, 667797
Committed: https://crrev.com/794079041f3fb283eb888f48c0d806b5427a634a
Cr-Commit-Position: refs/heads/master@{#434826}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Response to comments, fix tests #
Total comments: 1
Patch Set 3 : Remove debug lines #
Messages
Total messages: 18 (11 generated)
The CQ bit was checked by mmenke@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...
mmenke@chromium.org changed reviewers: + eroman@chromium.org
These overflows seem pretty benign, though I suppose you could imaging all sorts of weird/broken filters and middle boxes. Think there's only one more of these FTP issues left, at the moment, in the time parsing code. https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:138: epsv_line[start + 1] != epsv_line[start + 3]) { This is still ugly, but it seems a bit better to me than using raw pointers with null checks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:132: if (start == epsv_line.npos || epsv_line.length() - start < 6) This is more commonly written as |StringPiece::npos|, can you switch to that style? Also, may as well change the bound to 7 since that is the minimum for success it looks like. https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:138: epsv_line[start + 1] != epsv_line[start + 3]) { On 2016/11/28 22:28:18, mmenke wrote: > This is still ugly, but it seems a bit better to me than using raw pointers with > null checks. Can you extract epsv_line[start+1] to a variable? https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:146: size_t end = epsv_line.find(epsv_line[start - 1], start); Same comment. Rather than using epsv_line[start-1] please use a common variable for "<d>". https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:147: if (end == epsv_line.npos) StringPiece::npos https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:201: if (!ParseInt32(pieces[4], ParseIntFormat::NON_NEGATIVE, &p0)) ParseUint32() ? or should we add a ParseUint8() ? https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:205: if (p0 >= 256 || p1 >= 256) optional nit: I think it would be clearer writte as "p0 > 0xFF || p1 > 0xFF"
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:132: if (start == epsv_line.npos || epsv_line.length() - start < 6) On 2016/11/28 22:51:57, eroman (slow) wrote: > This is more commonly written as |StringPiece::npos|, can you switch to that > style? Also, may as well change the bound to 7 since that is the minimum for > success it looks like. Done. https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:138: epsv_line[start + 1] != epsv_line[start + 3]) { On 2016/11/28 22:51:57, eroman (slow) wrote: > On 2016/11/28 22:28:18, mmenke wrote: > > This is still ugly, but it seems a bit better to me than using raw pointers > with > > null checks. > > Can you extract epsv_line[start+1] to a variable? Done. https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:146: size_t end = epsv_line.find(epsv_line[start - 1], start); On 2016/11/28 22:51:57, eroman (slow) wrote: > Same comment. Rather than using epsv_line[start-1] please use a common variable > for "<d>". Done. https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:147: if (end == epsv_line.npos) On 2016/11/28 22:51:57, eroman (slow) wrote: > StringPiece::npos Done. https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:201: if (!ParseInt32(pieces[4], ParseIntFormat::NON_NEGATIVE, &p0)) On 2016/11/28 22:51:57, eroman (slow) wrote: > ParseUint32() ? > > or should we add a ParseUint8() ? Went with ParseUint32 (Don't think adding a new method is worth it, unless there are other places that need it) https://codereview.chromium.org/2539583002/diff/1/net/ftp/ftp_network_transac... net/ftp/ftp_network_transaction.cc:205: if (p0 >= 256 || p1 >= 256) On 2016/11/28 22:51:57, eroman (slow) wrote: > optional nit: I think it would be clearer writte as "p0 > 0xFF || p1 > 0xFF" Done.
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 checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2539583002/#ps40001 (title: "Remove debug lines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2539583002/diff/20001/net/ftp/ftp_network_tra... File net/ftp/ftp_network_transaction.cc (right): https://codereview.chromium.org/2539583002/diff/20001/net/ftp/ftp_network_tra... net/ftp/ftp_network_transaction.cc:138: if (isdigit(separator) || epsv_line[start + 2] != separator || I am not sure that we should be using isdigit() here. From consulting various documentation, it sounds like std::isdigit() is *generally* locale-insensitive, but not necessarily. Some implementations may recognize additional characters as digits :( I think it would be better to just test [0-9] directly. Or I guess could even call ParseUint32() on the single-character string piece for consistency. Generally speaking I don't think we should ever be using c library functions (which depend on the current locale) when parsing network protocols
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480375122822610,
"parent_rev": "c44e865cc57c4e71634416aeaecb0bdd62d63cd0", "commit_rev":
"f3a255059e36e7fc149998b31655de66bb3535a4"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use overflow-safe string-to-int parsing methods for FTP ports. BUG=667779,667797 ========== to ========== Use overflow-safe string-to-int parsing methods for FTP ports. BUG=667779,667797 Committed: https://crrev.com/794079041f3fb283eb888f48c0d806b5427a634a Cr-Commit-Position: refs/heads/master@{#434826} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/794079041f3fb283eb888f48c0d806b5427a634a Cr-Commit-Position: refs/heads/master@{#434826} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
