Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflow with massive character substitution #1823

Closed
saper opened this issue Jan 3, 2016 · 8 comments
Closed

Stack overflow with massive character substitution #1823

saper opened this issue Jan 3, 2016 · 8 comments

Comments

@saper
Copy link
Member

saper commented Jan 3, 2016

(from sass/node-sass#1289)

If we try do perform large character substitution on the 1688 byte SVG file (https://gist.github.com/xzyfer/2cc0fcfc3c7c0fdc061e) we consume more than 512 kilobytes on stack, which hits thread stack size limit on Mac OS X.

The backtrace is here: https://gist.github.com/saper/4cae7f96bd4bacb61d3d

To reproduce on other platforms one needs to limit the stack for the process (using limit stacksize 512 on C shells or ulimit -s 512 on Bourne shells).

The problem is the number of recursed calls to Sass::Eval::operator() - the topmost function usually fails when trying to access its own local variable space freshly allocated on the stack.

@mgreter
Copy link
Contributor

mgreter commented Jan 3, 2016

I don't see much we can do here, as the implementation of the custom function str-replace is actually recursive. I guess this was mainly done as sass seems to lack an offset argument for str-index. But the function can still be implemented with one loop only, altough it needs a bit more logic then.

@function str-replace($string, $search, $replace: '') {
  @if $search == '' {
    @return $string;
  }
  $remainder: $string;
  $string: '';
  $index: str-index($remainder, $search);
  @while $index {
    $string: $string + str-slice($remainder, 1, $index - 1) + $replace;
    $remainder: str-slice($remainder, $index + str-length($search));
    $index: str-index($remainder, $search);
  }
  @return $string + $remainder;
}

I have not really tested this much, but seems to yield the same results as the recursive implementation.

@mgreter
Copy link
Contributor

mgreter commented Jan 5, 2016

To repeat: this would have been much easier to implement if ruby sass would accept an optional offset for str-index (as pretty much all programming languages offer). Maybe it would be a nice addition and worth mentioning to the ruby sass team (//CC @chriseppstein @nex3).

@xzyfer
Copy link
Contributor

xzyfer commented Jan 5, 2016

It's worth creating an issue there to discuss.
On 6 Jan 2016 1:04 am, "Marcel Greter" notifications@github.com wrote:

To repeat: this would have been much easier to implement if ruby sass
would accept an optional offset for str-index (as pretty much all
programming languages offer). Maybe it would be a nice addition and worth
mentioning to the ruby sass team (//CC @chriseppstein
https://github.com/chriseppstein @nex3 https://github.com/nex3).


Reply to this email directly or view it on GitHub
#1823 (comment).

@mgreter
Copy link
Contributor

mgreter commented Jan 6, 2016

Opened sass/sass#1955. Closing this as no action will be taken from our side.

@mgreter mgreter closed this as completed Jan 6, 2016
@jakob-e
Copy link

jakob-e commented Feb 12, 2016

@saper – I had the same problem solved by chunking up the string (2000 characters each)
Encode SVG and a test (61443 characters)

//
//  Function to create an optimized svg url
//
@function svg-url($svg){
    //
    //  Add missing namespace
    //
    @if not str-index($svg, xmlns) {
        $svg: str-replace($svg, '<svg','<svg xmlns="http://www.w3.org/2000/svg"');   
    }    
    //    
    //  Chunk up string in order to avoid 
    //  "stack level too deep" error
    //     
    $encoded:'';
    $slice: 2000;
    $index: 0;
    $loops: ceil(str-length($svg)/$slice);
    @for $i from 1 through $loops {
        $chunk: str-slice($svg, $index, $index + $slice - 1); 
        //
        //   Encode (may need a few extra replacements)
        //
        $chunk: str-replace($chunk,'"','\'');
        $chunk: str-replace($chunk,'<','%3C');
        $chunk: str-replace($chunk,'>','%3E');
        $chunk: str-replace($chunk,'&','%26');
        $chunk: str-replace($chunk,'#','%23');       
        $encoded: #{$encoded}#{$chunk};
        $index: $index + $slice; 
    }
    @return url("data:image/svg+xml;charset=utf8,#{$encoded}");   
}

//  Helper function to replace characters in a string
@function str-replace($string, $search, $replace: '') {
    $index: str-index($string, $search); 
    @if $index { 
        @return str-slice($string, 1, $index - 1) + $replace + 
            str-replace(str-slice($string, $index + 
            str-length($search)), $search, $replace); 
    }
    @return $string; 
}

// Use It
.class { 
    background-image: svg-url('<svg xmlns="http://www.w3.org/2000/svg">.....</svg>');    
}

@matthias-vogt
Copy link

@jakob-e It's an edge case, but if the string gets split exactly where a theoretical match of a substring would be, that match won't be replaced. Fixing that needs some creativity 😁

@jakob-e
Copy link

jakob-e commented Sep 6, 2016

@matthias-vogt
Except from the first "add missing namespace" (note the edit) only single characters are replaced – why it shouldn't be a problem :-)

@matthias-vogt
Copy link

@jakob-e Just for anyone looking for a generic string-replace function. I wouldn't want anyone to have to debug that :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants