Stack overflow in agg (rasterizer_cells_aa) #4970

Closed
szekerest opened this Issue Aug 11, 2014 · 2 comments

Comments

Projects
None yet
2 participants
@szekerest
Member

szekerest commented Aug 11, 2014

I got a crashdump from a client which contains a stack overflow in the following location:

FAULTING_IP:
libmap!mapserver::rasterizer_cells_aamapserver::cell_aa::line+9b [e:\builds\mapserver-dev\renderers\agg\include\agg_rasterizer_cells_aa.h @ 327]
000000f0`07c2f6db 448b9c24c0000000 mov r11d,dword ptr [rsp+0C0h]

EXCEPTION_RECORD: ffffffffffffffff -- (.exr 0xffffffffffffffff)
ExceptionAddress: 000000f007c31498 (libmap!mapserver::rasterizer_cells_aamapserver::cell_aa::render_hline+0x0000000000000068)
ExceptionCode: c00000fd (Stack overflow)
ExceptionFlags: 00000000
NumberParameters: 2
Parameter[0]: 0000000000000001
Parameter[1]: 000000f005095fe8

FAULTING_SOURCE_CODE:
323: {
324: int cx = (x1 + x2) >> 1;
325: int cy = (y1 + y2) >> 1;
326: line(x1, y1, cx, cy);

327: line(cx, cy, x2, y2);
328: }
329:
330: int dy = y2 - y1;
331: int ex1 = x1 >> poly_subpixel_shift;
332: int ex2 = x2 >> poly_subpixel_shift;

The issue seems to be the same as what has been reported for mapnik earlier:

mapnik/mapnik#253

And the corresponding fix was:

if(dx >= dx_limit || dx <= -dx_limit)
{
int cx = (x1 + x2) >> 1;
int cy = (y1 + y2) >> 1;

        // Bail if values are so large they are likely to wrap
        if ((std::abs(x1) >= std::numeric_limits<int>::max()/2) || (std::abs(y1) >= std::numeric_limits<int>::max()/2) ||
            (std::abs(x2) >= std::numeric_limits<int>::max()/2) || (std::abs(y2) >= std::numeric_limits<int>::max()/2))
                return;

        line(x1, y1, cx, cy);
        line(cx, cy, x2, y2);
    }
@szekerest

This comment has been minimized.

Show comment
Hide comment
@szekerest

szekerest Aug 13, 2014

Member

Tested the proposed solution but it didn't work.

Instead we have to implement integer overflow safe calculations like this:

    enum dx_limit_e { dx_limit = 16384 << poly_subpixel_shift };

    // Checking the limit and avoid integer overflows
    if(((x1 < x2)? ((x1 < 0)? (x1 + dx_limit <= x2) : (x1 <= x2 - dx_limit)) : 
                  ((x2 < 0)? (x2 + dx_limit <= x1) : (x2 <= x1 - dx_limit))) ||
       ((y1 < y2)? ((y1 < 0)? (y1 + dx_limit <= y2) : (y1 <= y2 - dx_limit)) : 
                  ((y2 < 0)? (y2 + dx_limit <= y1) : (y2 <= y1 - dx_limit))))
    {
        int cx = (x2&x1) + (x2^x1)/2;
        int cy = (y2&y1) + (y2^y1)/2;

        line(x1, y1, cx, cy);
        line(cx, cy, x2, y2);
    }
Member

szekerest commented Aug 13, 2014

Tested the proposed solution but it didn't work.

Instead we have to implement integer overflow safe calculations like this:

    enum dx_limit_e { dx_limit = 16384 << poly_subpixel_shift };

    // Checking the limit and avoid integer overflows
    if(((x1 < x2)? ((x1 < 0)? (x1 + dx_limit <= x2) : (x1 <= x2 - dx_limit)) : 
                  ((x2 < 0)? (x2 + dx_limit <= x1) : (x2 <= x1 - dx_limit))) ||
       ((y1 < y2)? ((y1 < 0)? (y1 + dx_limit <= y2) : (y1 <= y2 - dx_limit)) : 
                  ((y2 < 0)? (y2 + dx_limit <= y1) : (y2 <= y1 - dx_limit))))
    {
        int cx = (x2&x1) + (x2^x1)/2;
        int cy = (y2&y1) + (y2^y1)/2;

        line(x1, y1, cx, cy);
        line(cx, cy, x2, y2);
    }
@jratike80

This comment has been minimized.

Show comment
Hide comment
@jratike80

jratike80 Nov 2, 2015

@szekerest , can you confirm that your fix was good and close the ticket?

@szekerest , can you confirm that your fix was good and close the ticket?

@szekerest szekerest closed this Nov 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment