Skip Menu |

This queue is for tickets about the JavaScript-Minifier-XS CPAN distribution.

Report information
The Basics
Id: 58416
Status: resolved
Priority: 0/
Queue: JavaScript-Minifier-XS

People
Owner: Nobody in particular
Requestors: haarg [...] haarg.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: 0.07
Fixed in: 0.11



Subject: Minifying non-javascript text crashes perl
If you try to minify some text that is not JavaScript, perl crashes. Attached is a test to show this behavior and attempt to verify the original text would be returned in such a case.
Subject: minify_non_javascript.t
use strict; use warnings; use Test::More tests => 1; use JavaScript::Minifier::XS qw(minify); use POSIX (); unless (fork) { eval { my $original = 'not javascript'; my $minified = minify($original); if ($minified eq $original) { POSIX::_exit(0); } else { diag "wanted '$original', got '$minified'"; POSIX::_exit(1); } }; diag "Minifying died"; POSIX::_exit(2); } wait; ok $? == 0, "minifying non-javascript gives original content back and doesn't crash" or do { $? & 127 and diag "Core dump" };
From: colinmkeith [...] gmail.com
On Tue Jun 15 13:15:06 2010, haarg wrote: Show quoted text
> If you try to minify some text that is not JavaScript, perl crashes. > Attached is a test to show this behavior and attempt to verify the > original text would be returned in such a case.
I simplified this to just: %perl -MJavaScript::Minifier::XS=minify -e 'print minify(q(test));' The issue is that the helper macros assume that "node" has a value; #define nodeIsWHITESPACE(node) ((node->type == NODE_WHITESPACE)) But for the last node in the tree the "next" node is NULL and so querying a member of the structure, here node->type, generates a segfault. The solution is to check if node != NULL: #define nodeIsWHITESPACE(node) ((node != NULL) && (node->type == NODE_WHITESPACE)) Please apply the patch that is attached.
Subject: JavaScript_Minifier_XS_xs.patch
--- lib/JavaScript/Minifier/XS.xs.orig 2011-12-07 21:49:22.000000000 -0500 +++ lib/JavaScript/Minifier/XS.xs 2011-12-07 21:50:13.000000000 -0500 @@ -172,14 +172,14 @@ } /* macros to help see what kind of node we've got */ -#define nodeIsWHITESPACE(node) ((node->type == NODE_WHITESPACE)) -#define nodeIsBLOCKCOMMENT(node) ((node->type == NODE_BLOCKCOMMENT)) -#define nodeIsLINECOMMENT(node) ((node->type == NODE_LINECOMMENT)) -#define nodeIsIDENTIFIER(node) ((node->type == NODE_IDENTIFIER)) -#define nodeIsLITERAL(node) ((node->type == NODE_LITERAL)) -#define nodeIsSIGIL(node) ((node->type == NODE_SIGIL)) +#define nodeIsWHITESPACE(node) ((node != NULL) && (node->type == NODE_WHITESPACE)) +#define nodeIsBLOCKCOMMENT(node) ((node != NULL) && (node->type == NODE_BLOCKCOMMENT)) +#define nodeIsLINECOMMENT(node) ((node != NULL) && (node->type == NODE_LINECOMMENT)) +#define nodeIsIDENTIFIER(node) ((node != NULL) && (node->type == NODE_IDENTIFIER)) +#define nodeIsLITERAL(node) ((node != NULL) && (node->type == NODE_LITERAL)) +#define nodeIsSIGIL(node) ((node != NULL) && (node->type == NODE_SIGIL)) -#define nodeIsEMPTY(node) ((node->type == NODE_EMPTY) || (node->length==0) || (node->contents=NULL)) +#define nodeIsEMPTY(node) ((node != NULL) && (node->type == NODE_EMPTY) || (node->length==0) || (node->contents=NULL)) #define nodeIsCOMMENT(node) (nodeIsBLOCKCOMMENT(node) || nodeIsLINECOMMENT(node)) #define nodeIsIECONDITIONALBLOCKCOMMENT(node) (nodeIsBLOCKCOMMENT(node) && nodeBeginsWith(node,"/*@") && nodeEndsWith(node,"@*/")) #define nodeIsIECONDITIONALLINECOMMENT(node) (nodeIsLINECOMMENT(node) && nodeBeginsWith(node,"//@")) @@ -187,7 +187,7 @@ #define nodeIsPREFIXSIGIL(node) (nodeIsSIGIL(node) && charIsPrefix(node->contents[0])) #define nodeIsPOSTFIXSIGIL(node) (nodeIsSIGIL(node) && charIsPostfix(node->contents[0])) #define nodeIsENDSPACE(node) (nodeIsWHITESPACE(node) && charIsEndspace(node->contents[0])) -#define nodeIsCHAR(node,ch) ((node->contents[0]==ch) && (node->length==1)) +#define nodeIsCHAR(node,ch) ((node != NULL) && (node->contents[0]==ch) && (node->length==1)) /* **************************************************************************** * NODE MANIPULATION FUNCTIONS
From: colinmkeith [...] gmail.com
Corrected a logic issue in nodeIsEMPTY() macro. Please use this second patch in favour of the first.
Subject: JavaScript_Minifier_XS_xs.patch
--- lib/JavaScript/Minifier/XS.xs.orig 2011-12-07 21:49:22.000000000 -0500 +++ lib/JavaScript/Minifier/XS.xs 2011-12-07 21:50:13.000000000 -0500 @@ -172,14 +172,14 @@ } /* macros to help see what kind of node we've got */ -#define nodeIsWHITESPACE(node) ((node->type == NODE_WHITESPACE)) -#define nodeIsBLOCKCOMMENT(node) ((node->type == NODE_BLOCKCOMMENT)) -#define nodeIsLINECOMMENT(node) ((node->type == NODE_LINECOMMENT)) -#define nodeIsIDENTIFIER(node) ((node->type == NODE_IDENTIFIER)) -#define nodeIsLITERAL(node) ((node->type == NODE_LITERAL)) -#define nodeIsSIGIL(node) ((node->type == NODE_SIGIL)) +#define nodeIsWHITESPACE(node) ((node != NULL) && (node->type == NODE_WHITESPACE)) +#define nodeIsBLOCKCOMMENT(node) ((node != NULL) && (node->type == NODE_BLOCKCOMMENT)) +#define nodeIsLINECOMMENT(node) ((node != NULL) && (node->type == NODE_LINECOMMENT)) +#define nodeIsIDENTIFIER(node) ((node != NULL) && (node->type == NODE_IDENTIFIER)) +#define nodeIsLITERAL(node) ((node != NULL) && (node->type == NODE_LITERAL)) +#define nodeIsSIGIL(node) ((node != NULL) && (node->type == NODE_SIGIL)) -#define nodeIsEMPTY(node) ((node->type == NODE_EMPTY) || (node->length==0) || (node->contents=NULL)) +#define nodeIsEMPTY(node) ((node != NULL) && ((node->type == NODE_EMPTY) || (node->length==0) || (node->contents=NULL))) #define nodeIsCOMMENT(node) (nodeIsBLOCKCOMMENT(node) || nodeIsLINECOMMENT(node)) #define nodeIsIECONDITIONALBLOCKCOMMENT(node) (nodeIsBLOCKCOMMENT(node) && nodeBeginsWith(node,"/*@") && nodeEndsWith(node,"@*/")) #define nodeIsIECONDITIONALLINECOMMENT(node) (nodeIsLINECOMMENT(node) && nodeBeginsWith(node,"//@")) @@ -187,7 +187,7 @@ #define nodeIsPREFIXSIGIL(node) (nodeIsSIGIL(node) && charIsPrefix(node->contents[0])) #define nodeIsPOSTFIXSIGIL(node) (nodeIsSIGIL(node) && charIsPostfix(node->contents[0])) #define nodeIsENDSPACE(node) (nodeIsWHITESPACE(node) && charIsEndspace(node->contents[0])) -#define nodeIsCHAR(node,ch) ((node->contents[0]==ch) && (node->length==1)) +#define nodeIsCHAR(node,ch) ((node != NULL) && (node->contents[0]==ch) && (node->length==1)) /* **************************************************************************** * NODE MANIPULATION FUNCTIONS
On Tue Jun 15 13:15:06 2010, haarg wrote: Show quoted text
> If you try to minify some text that is not JavaScript, perl crashes. > Attached is a test to show this behavior and attempt to verify the > original text would be returned in such a case.
Perhaps I'm missing something, but why would you be trying to minify non-JS code with a JS minifier? e.g. if you know its not JS, why are you trying to minify it?
On Tue Jan 27 12:31:04 2015, GTERMARS wrote: Show quoted text
> On Tue Jun 15 13:15:06 2010, haarg wrote:
> > If you try to minify some text that is not JavaScript, perl crashes. > > Attached is a test to show this behavior and attempt to verify the > > original text would be returned in such a case.
> > Perhaps I'm missing something, but why would you be trying to minify > non-JS code with a JS minifier? e.g. if you know its not JS, why are > you trying to minify it?
User input. You may not be certain ahead of time that you are dealing with valid javascript. In this case, broken output is acceptable, since the input was broken. But crashing perl is never acceptable, no matter the input.
On Tue Jan 27 16:03:15 2015, haarg wrote: Show quoted text
> In this case, broken output is acceptable, since the input > was broken. But crashing perl is never acceptable, no > matter the input.
True enough.
As of v0.11, this should no longer crash Perl. As you noted, there's no guarantee that what you get back is sane, but this new version should react better to the horrors of "whatever input we got from the user". If, however, you find other JS that causes it to go boom, please *do* re-open the ticket or create a new one (I promise to get to it a whole lot quicker this time around).