<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - DoS attack based on using SVG to generate invalid pointers from a _cairo_image_surface in write_png" href="https://bugs.freedesktop.org/show_bug.cgi?id=98165#c8">Comment # 8</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - DoS attack based on using SVG to generate invalid pointers from a _cairo_image_surface in write_png" href="https://bugs.freedesktop.org/show_bug.cgi?id=98165">bug 98165</a> from <a class="email" href="mailto:jbowler@acm.org" title="John Bowler <jbowler@acm.org>"> John Bowler</a> <pre>(In reply to Seth Arnold from <a href="show_bug.cgi?id=98165#c7">comment #7</a>) > stride = cairo_format_stride_for_width(format, width); I think that function should return a ptrdiff_t. If it does so the compiler will start issuing warnings on 64-bit builds where the result is truncated to 32 bits, as in the above assignment. I believe that in general it is ok for 'width' and 'height' to be (int), assuming you don't support 16-bit systems, but any multiplication has to be done with care: > ptr = malloc (stride* height); That strikes me as bogus. Images with rows as long as 2^31 bytes are rare; pretty much limited to people trying to break libpng! However images with 2^31 or more bytes in total can happen and it is perfectly possible on a modern desktop/laptop/tablet to end up with one in memory. (Maybe it's a little dumb, but it is possible.) In any case if cairo_format_stride_for_width uses (-1) as an error return (I'm not that was what you are saying) the callers need to check for it. > surface = cairo_image_surface_create_for_data (ptr, format, > width, height, stride); The last (stride) parameter should have type ptrdiff_t. I think the point I was trying to make before is that if the change is done in the struct and places that use the 'stride' member it will force a lot of other changes (int to ptrdiff_t) but a GNU 64-bit compiler with -Wall -Wextra should show the vast majority of the issues. > Here's another example: > > static cairo_status_t > _cairo_image_spans_and_zero (void *abstract_renderer, > int y, int height, > const cairo_half_open_span_t *spans, > unsigned num_spans) > { > cairo_image_span_renderer_t *r = abstract_renderer; > uint8_t *mask; > int len; > > mask = r->u.mask.data; > if (y > r->u.mask.extents.y) { > len = (y - r->u.mask.extents.y) * r->u.mask.stride; > memset (mask, 0, len); > mask += len; > } > > > The u.mask.stride would be a ptrdiff_t after this patch, but 'len' is still > only an 'int'. Right; that should result in a compiler warning for the assignment to 'len', so then the next step is to change 'len' to ptrdiff_t. Fixing the warnings forces the change through to all the required places. The only way to stop the warning without fixing the problem is to use an explicit cast; a static_cast in C++ terms. casts are evil ;-)</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the QA Contact for the bug.</li> </ul> </body> </html>