Check for overflow when merging text nodes.

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/+/14b7c024aaec338adcbf87cbeee54cf6137d7f8a

[dominicc (has gone to gerrit), 2017-01-11 08:51:55]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 08:52:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-01-11 09:03:18]

dominicc@chromium.org changed reviewers:
+ scottmg@chromium.org

[dominicc (has gone to gerrit), 2017-01-11 09:03:42]

Description was changed from

==========
Check for overflow when merging text nodes.

BUG=676623
==========

to

==========
xsltAddTextString: Check for overflow when merging text nodes.

BUG=676623
==========

[dominicc (has gone to gerrit), 2017-01-11 09:03:50]

PTAL

[dominicc (has gone to gerrit), 2017-01-11 09:04:01]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 09:04:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 10:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 10:02:41]

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_...)

[scottmg, 2017-01-11 17:46:07]

(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.

[scottmg, 2017-01-11 18:02:23]

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.

[dominicc (has gone to gerrit), 2017-01-13 04:26:57]

Patch from Nick Wellnhofer upstream.

[dominicc (has gone to gerrit), 2017-01-13 04:26:59]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 04:27:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2017-01-13 04:28:56]

PTAL

Nick contributed this simpler, better patch at
https://bugzilla.gnome.org/show_bug.cgi?id=777124

[dominicc (has gone to gerrit), 2017-01-13 04:32:16]

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.

[scottmg, 2017-01-13 04:57:02]

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.

[commit-bot: I haz the power, 2017-01-13 05:12:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 05:12:49]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2017-01-13 05:49:23]

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.

[dominicc (has gone to gerrit), 2017-01-24 09:11:55]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2017-01-24 09:12:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 09:13:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 09:13:46]

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...)

[dominicc (has gone to gerrit), 2017-01-25 08:48:19]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2017-01-25 08:48:19]

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.")

[commit-bot: I haz the power, 2017-01-25 08:48:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 09:50:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485334099356960,
"parent_rev": "e29607bd33d91db8c58127eb6469fca2ad22c16a", "commit_rev":
"14b7c024aaec338adcbf87cbeee54cf6137d7f8a"}

[commit-bot: I haz the power, 2017-01-25 09:50:53]

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...
==========

[commit-bot: I haz the power, 2017-01-25 09:50:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/14b7c024aaec338adcbf87cbeee5...