Bug #107900 for Imager: Attempt to extend stack by negative value causes panic with perl 5.23.4

Wed Oct 21 09:40:02 2015 nanis [...] runu.moc.invalid - Ticket created

Subject:

Attempt to extend stack by negative value causes panic with perl 5.23.4

[_get_anonymous_color_histo in Imager.xs][1] calls [_get_anonymous_color_histo in image.c][2]. The latter routine [returns -1][3] if the number of colors exceeds the `maxc` parameter. In Imager.xs, that return value is used to [extend the stack][4]. Using `EXTEND` with a negative amount causes a recently committed check to be hit in [`scope.c`][5] (see also [diff][6]). This causes a failure in [t/100-base/030-count.t][7]: @colour_usage = $im->getcolorusage(maxcolors => 2); is(@colour_usage, 0, 'test overflow check'); The invocation of `getcolorusage` with the maxcolors argument causes a panic, and the test is not reached. Arguably, the underlying `EXTEND` with a negative argument has always been wrong. I have not investigated the behavior of `EXTEND` through releases, but, logically, adjusting the return stack by a negative value seems like it could not have a legitimate use. I will submit a patch when I get a chance to look at this again. Thank you. -- Sinan [1]: https://metacpan.org/source/TONYC/Imager-1.003/Imager.xs#L2668 [2]: https://metacpan.org/source/TONYC/Imager-1.003/image.c#L1273 [3]: https://metacpan.org/source/TONYC/Imager-1.003/image.c#L1303 [4]: https://metacpan.org/source/TONYC/Imager-1.003/Imager.xs#L2677 [5]: http://perl5.git.perl.org/perl.git/commit/6768377c79109b7124f0c8a4e3677982689d9f49?f=scope.c [6]: http://perl5.git.perl.org/perl.git/blobdiff/73e8ff0004522621dfb42f01966853b51d5522a6..6768377c79109b7124f0c8a4e3677982689d9f49:/scope.c [7]: https://metacpan.org/source/TONYC/Imager-1.003/t/100-base/030-countc.t#L48

Wed Oct 21 17:39:32 2015 nanis [...] runu.moc.invalid - Correspondence added

On Wed Oct 21 09:40:02 2015, NANIS wrote: Show quoted text

> I will submit a patch when I get a chance to look at this again.

Patch attached.

Subject:

0001-RT-107900-Panic-due-to-attempt-to-extend-stack-by-ne.patch

From 5dd8192eac1eed92a5112767a0b23b3935b7efd7 Mon Sep 17 00:00:00 2001 From: "A. Sinan Unur" <nanis@cpan.org> Date: Wed, 21 Oct 2015 15:41:24 -0400 Subject: [PATCH] RT #107900 Panic due to attempt to extend stack by negative amount See https://rt.cpan.org/Public/Bug/Display.html?id=107900 See also http://www.cpantesters.org/distro/I/Imager.html#Imager-1.003?grade=1&perlmat=3&patches=2&oncpan=2&distmat=2&perlver=5.23.4&osname=ALL&version=1.003 Fix by only EXTENDing return stack and pushing values if returned col_cnt is positive. --- Imager.xs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Imager.xs b/Imager.xs index 6a205ba..6803f04 100644 --- a/Imager.xs +++ b/Imager.xs @@ -2674,12 +2674,17 @@ i_get_anonymous_color_histo(im, maxc = 0x40000000) int col_cnt; PPCODE: col_cnt = i_get_anonymous_color_histo(im, &col_usage, maxc); + if (col_cnt > 0) { EXTEND(SP, col_cnt); for (i = 0; i < col_cnt; i++) { PUSHs(sv_2mortal(newSViv( col_usage[i]))); } myfree(col_usage); XSRETURN(col_cnt); + } + else { + XSRETURN_EMPTY; + } void -- 1.9.5.msysgit.0

Wed Oct 21 17:39:33 2015 nanis [...] runu.moc.invalid - Status changed from 'new' to 'open'

Wed Oct 21 17:49:06 2015 tony [...] develop-help.com - Correspondence added

CC:	;
Subject:	Re: [rt.cpan.org #107900] Attempt to extend stack by negative value causes panic with perl 5.23.4
Date:	Thu, 22 Oct 2015 08:48:55 +1100
To:	"A. Sinan Unur via RT" <bug-Imager [...] rt.cpan.org>
From:	Tony Cook <tony [...] develop-help.com>

On Wed, Oct 21, 2015 at 09:40:03AM -0400, A. Sinan Unur via RT wrote: Show quoted text

> [_get_anonymous_color_histo in Imager.xs][1] calls [_get_anonymous_color_histo in image.c][2]. > > The latter routine [returns -1][3] if the number of colors exceeds the `maxc` parameter. > > In Imager.xs, that return value is used to [extend the stack][4]. > > Using `EXTEND` with a negative amount causes a recently committed check to be hit in [`scope.c`][5] (see also [diff][6]). > > This causes a failure in [t/100-base/030-count.t][7]: > > @colour_usage = $im->getcolorusage(maxcolors => 2); > is(@colour_usage, 0, 'test overflow check'); > > The invocation of `getcolorusage` with the maxcolors argument causes a panic, and the test is not reached. > > Arguably, the underlying `EXTEND` with a negative argument has always been wrong. I have not investigated the behavior of `EXTEND` through releases, but, logically, adjusting the return stack by a negative value seems like it could not have a legitimate use. > > I will submit a patch when I get a chance to look at this again.

I think the solution is to return an empty list when i_get_anonymous_color_histo() returns -1. Tony

Wed Nov 04 06:12:12 2015 TONYC [...] cpan.org - Status changed from 'open' to 'patched'

Sat Feb 20 19:42:32 2016 TONYC [...] cpan.org - Correspondence added

On Wed Oct 21 17:39:32 2015, NANIS wrote: Show quoted text

> On Wed Oct 21 09:40:02 2015, NANIS wrote:

> > I will submit a patch when I get a chance to look at this again.

> > Patch attached.

Thanks, this was applied as 8e273c14def50a167fdfc5837aa2208a7b2ff6f0 and included in Imager 1.004 (a few months ago.) Tony

Sat Feb 20 19:42:47 2016 TONYC [...] cpan.org - Status changed from 'patched' to 'resolved'