|
|
Created:
3 years, 11 months ago by dominicc (has gone to gerrit) Modified:
3 years, 11 months ago Reviewers:
scottmg CC:
chromium-reviews, dominicc+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionxsltAddTextString: Check for overflow when merging text nodes.
BUG=676623
Review-Url: https://codereview.chromium.org/2626983002
Cr-Commit-Position: refs/heads/master@{#445987}
Committed: https://chromium.googlesource.com/chromium/src/+/14b7c024aaec338adcbf87cbeee54cf6137d7f8a
Patch Set 1 #
Total comments: 3
Patch Set 2 : Patch from Nick Wellnhofer upstream. #
Total comments: 2
Patch Set 3 : Rebase on top of recent roll. #
Messages
Total messages: 30 (18 generated)
The CQ bit was checked by dominicc@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...
dominicc@chromium.org changed reviewers: + scottmg@chromium.org
Description was changed from ========== Check for overflow when merging text nodes. BUG=676623 ========== to ========== xsltAddTextString: Check for overflow when merging text nodes. BUG=676623 ==========
PTAL
The CQ bit was checked by dominicc@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
(I can't access the bug, maybe that'd clear things up for me.) https://codereview.chromium.org/2626983002/diff/1/third_party/libxslt/libxslt... File third_party/libxslt/libxslt/transform.c (right): https://codereview.chromium.org/2626983002/diff/1/third_party/libxslt/libxslt... third_party/libxslt/libxslt/transform.c:828: if (newbuf == NULL || size < ctxt->lasttsize) { This is intended to be an overflow check, or? (nominally we set size to (lasttsize + len + 100) ^ 2). lasttsize is unsigned int, but then we're calculating in a size_t... so I don't understand how size could be < lasttsize. (Apologies if I'm being dumb.) https://codereview.chromium.org/2626983002/diff/1/third_party/libxslt/libxslt... third_party/libxslt/libxslt/transform.c:837: if (ctxt->lasttuse >= ctxt->lasttsize - len) { nit; This indent looks odd in Rietveld, but maybe it's fine with the right tab settings.
https://codereview.chromium.org/2626983002/diff/1/third_party/libxslt/libxslt... File third_party/libxslt/libxslt/transform.c (right): https://codereview.chromium.org/2626983002/diff/1/third_party/libxslt/libxslt... third_party/libxslt/libxslt/transform.c:828: if (newbuf == NULL || size < ctxt->lasttsize) { On 2017/01/11 17:46:07, scottmg wrote: > This is intended to be an overflow check, or? (nominally we set size to > (lasttsize + len + 100) ^ 2). > > lasttsize is unsigned int, but then we're calculating in a size_t... so I don't > understand how size could be < lasttsize. > > (Apologies if I'm being dumb.) (Er, * 2 above obviously) I guess on x86 it might make some sense if all the types are 32 bit. I was thinking x64, so size_t of 8b vs ctxt->lasttsize and len of 4b.
Patch from Nick Wellnhofer upstream.
The CQ bit was checked by dominicc@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 Nick contributed this simpler, better patch at https://bugzilla.gnome.org/show_bug.cgi?id=777124
Note, preferably the roll in https://codereview.chromium.org/2634473003 would land first and README.chromium is rebased on that, but landing this first and redoing the roll is also fine.
lgtm https://codereview.chromium.org/2626983002/diff/20001/third_party/libxslt/lib... File third_party/libxslt/libxslt/transform.c (right): https://codereview.chromium.org/2626983002/diff/20001/third_party/libxslt/lib... third_party/libxslt/libxslt/transform.c:835: /* Double buffer size but increase by at least 100 bytes. */ Doesn't look like there's any doubling going on now, but anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the quick review, you're up late/early. If you're feeling particularly sleepless or charitable could you take a look at https://codereview.chromium.org/2634473003/ ? https://codereview.chromium.org/2626983002/diff/20001/third_party/libxslt/lib... File third_party/libxslt/libxslt/transform.c (right): https://codereview.chromium.org/2626983002/diff/20001/third_party/libxslt/lib... third_party/libxslt/libxslt/transform.c:835: /* Double buffer size but increase by at least 100 bytes. */ On 2017/01/13 at 04:57:02, scottmg wrote: > Doesn't look like there's any doubling going on now, but anyway. I think the doubling works by copying the size to extra, and then adding extra to the size in line 843. So it doesn't exactly double the buffer, but resizes the buffer to double the required size.
The CQ bit was checked by dominicc@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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by dominicc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2626983002/#ps40001 (title: "Rebase on top of recent roll.")
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": 40001, "attempt_start_ts": 1485334099356960, "parent_rev": "e29607bd33d91db8c58127eb6469fca2ad22c16a", "commit_rev": "14b7c024aaec338adcbf87cbeee54cf6137d7f8a"}
Message was sent while issue was closed.
Description was changed from ========== xsltAddTextString: Check for overflow when merging text nodes. BUG=676623 ========== to ========== xsltAddTextString: Check for overflow when merging text nodes. BUG=676623 Review-Url: https://codereview.chromium.org/2626983002 Cr-Commit-Position: refs/heads/master@{#445987} Committed: https://chromium.googlesource.com/chromium/src/+/14b7c024aaec338adcbf87cbeee5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/14b7c024aaec338adcbf87cbeee5... |