Skip Menu |
 

This queue is for tickets about the Imager CPAN distribution.

Report information
The Basics
Id: 107900
Status: resolved
Priority: 0/
Queue: Imager

People
Owner: Nobody in particular
Requestors: nanis [...] runu.moc.invalid
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 1.003
Fixed in: (no value)

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



Subject: Attempt to extend stack by negative value causes panic with perl 5.23.4
Download (untitled) / with headers
text/plain 1.5k
[_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
Download (untitled) / with headers
text/plain 125b
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
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>
Download (untitled) / with headers
text/plain 1.1k
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
Download (untitled) / with headers
text/plain 300b
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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.