the following invalid read is reproducible in the trunk and icc branch, it might be associated with non deterministic results in the icc branch. valgrind --auto-run-dsymutil=yes ./gs -sDEVICE=pbmraw -r300 -sOutputFile=/dev/null ~/tests_private/comparefiles/Bug688807.pdf ==98218== Memcheck, a memory error detector. ==98218== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al. ==98218== Using LibVEX rev 1897, a library for dynamic binary translation. ==98218== Copyright (C) 2004-2009, and GNU GPL'd, by OpenWorks LLP. ==98218== Using valgrind-3.5.0.SVN, a dynamic binary instrumentation framework. ==98218== Copyright (C) 2000-2009, and GNU GPL'd, by Julian Seward et al. ==98218== For more details, rerun with: -v ==98218== GPL Ghostscript SVN PRE-RELEASE 8.72 (2010-02-11) Copyright (C) 2010 Artifex Software, Inc. All rights reserved. This software comes with NO WARRANTY: see the file PUBLIC for details. Processing pages 1 through 2. Page 1 Font --nostringval-- ( aliased from Verdana-BoldItalic ) is being rendered with FAPI=FreeType Font --nostringval-- ( aliased fro==98218== Invalid read of size 1 ==98218== at 0x115D31F: memcpy (mc_replace_strmem.c:482) ==98218== by 0x4C2AC9: bytes_copy_rectangle (gsbitops.c:657) ==98218== by 0x2971BA: cmd_put_bits (gxclbits.c:231) ==98218== by 0x2A94B9: clist_copy_mono (gxclrect.c:632) ==98218== by 0x509D75: clip_copy_mono (gxclip.c:401) ==98218== by 0x5147BE: gx_dc_pure_fill_masked (gxdcolor.c:444) ==98218== by 0x554E21: gx_default_fill_mask (gdevdbit.c:332) ==98218== by 0x2AE6B0: clist_fill_mask (gxclimag.c:102) ==98218== by 0x5395A1: gx_image_fill_masked (gximask.c:105) ==98218== by 0x1BFDB6: fapi_finish_render_aux (zfapi.c:1662) ==98218== by 0x1BFF67: fapi_finish_render (zfapi.c:1691) ==98218== by 0x1C27E2: FAPI_do_char (zfapi.c:2328) ==98218== Address 0x2b340ca is 0 bytes after a block of size 5,146 alloc'd ==98218== at 0x1159516: malloc (vg_replace_malloc.c:193) ==98218== by 0x4E79AA: gs_heap_alloc_bytes (gsmalloc.c:182) ==98218== by 0x4E7BAC: gs_heap_alloc_byte_array (gsmalloc.c:241) ==98218== by 0x1C3244: FF_alloc (fapi_ft.c:97) ==98218== by 0x1DA37A: ft_mem_qalloc (ftutil.c:76) ==98218== by 0x1DA314: ft_mem_alloc (ftutil.c:55) ==98218== by 0x1DA4D4: ft_mem_qrealloc (ftutil.c:145) ==98218== by 0x1DA3E1: ft_mem_realloc (ftutil.c:101) ==98218== by 0x212B48: ft_raster1_render (ftrend1.c:199) ==98218== by 0x1D5A81: FT_Render_Glyph_Internal (ftobjs.c:3812) ==98218== by 0x1D5B32: FT_Render_Glyph (ftobjs.c:3852) ==98218== by 0x1D1ADF: FT_Load_Glyph (ftobjs.c:781) ==98218== ==98218== Invalid read of size 1 ==98218== at 0x115D326: memcpy (mc_replace_strmem.c:482) ==98218== by 0x4C2AC9: bytes_copy_rectangle (gsbitops.c:657) ==98218== by 0x2971BA: cmd_put_bits (gxclbits.c:231) ==98218== by 0x2A94B9: clist_copy_mono (gxclrect.c:632) ==98218== by 0x509D75: clip_copy_mono (gxclip.c:401) ==98218== by 0x5147BE: gx_dc_pure_fill_masked (gxdcolor.c:444) ==98218== by 0x554E21: gx_default_fill_mask (gdevdbit.c:332) ==98218== by 0x2AE6B0: clist_fill_mask (gxclimag.c:102) ==98218== by 0x5395A1: gx_image_fill_masked (gximask.c:105) ==98218== by 0x1BFDB6: fapi_finish_render_aux (zfapi.c:1662) ==98218== by 0x1BFF67: fapi_finish_render (zfapi.c:1691) ==98218== by 0x1C27E2: FAPI_do_char (zfapi.c:2328) ==98218== Address 0x2b340cb is 1 bytes after a block of size 5,146 alloc'd ==98218== at 0x1159516: malloc (vg_replace_malloc.c:193) ==98218== by 0x4E79AA: gs_heap_alloc_bytes (gsmalloc.c:182) ==98218== by 0x4E7BAC: gs_heap_alloc_byte_array (gsmalloc.c:241) ==98218== by 0x1C3244: FF_alloc (fapi_ft.c:97) ==98218== by 0x1DA37A: ft_mem_qalloc (ftutil.c:76) ==98218== by 0x1DA314: ft_mem_alloc (ftutil.c:55) ==98218== by 0x1DA4D4: ft_mem_qrealloc (ftutil.c:145) ==98218== by 0x1DA3E1: ft_mem_realloc (ftutil.c:101) ==98218== by 0x212B48: ft_raster1_render (ftrend1.c:199) ==98218== by 0x1D5A81: FT_Render_Glyph_Internal (ftobjs.c:3812) ==98218== by 0x1D5B32: FT_Render_Glyph (ftobjs.c:3852) ==98218== by 0x1D1ADF: FT_Load_Glyph (ftobjs.c:781) ==98218== ==98218== Invalid read of size 1 ==98218== at 0x115D290: memcpy (mc_replace_strmem.c:482) ==98218== by 0x4C2AC9: bytes_copy_rectangle (gsbitops.c:657) ==98218== by 0x2971BA: cmd_put_bits (gxclbits.c:231) ==98218== by 0x2A94B9: clist_copy_mono (gxclrect.c:632) ==98218== by 0x509D75: clip_copy_mono (gxclip.c:401) ==98218== by 0x5147BE: gx_dc_pure_fill_masked (gxdcolor.c:444) ==98218== by 0x554E21: gx_default_fill_mask (gdevdbit.c:332) ==98218== by 0x2AE6B0: clist_fill_mask (gxclimag.c:102) ==98218== by 0x5395A1: gx_image_fill_masked (gximask.c:105) ==98218== by 0x1BFDB6: fapi_finish_render_aux (zfapi.c:1662) ==98218== by 0x1BFF67: fapi_finish_render (zfapi.c:1691) ==98218== by 0x1C27E2: FAPI_do_char (zfapi.c:2328) ==98218== Address 0x1cbd1cb is 1 bytes after a block of size 5,354 alloc'd ==98218== at 0x1159516: malloc (vg_replace_malloc.c:193) ==98218== by 0x4E79AA: gs_heap_alloc_bytes (gsmalloc.c:182) ==98218== by 0x4E7BAC: gs_heap_alloc_byte_array (gsmalloc.c:241) ==98218== by 0x1C3244: FF_alloc (fapi_ft.c:97) ==98218== by 0x1DA37A: ft_mem_qalloc (ftutil.c:76) ==98218== by 0x1DA314: ft_mem_alloc (ftutil.c:55) ==98218== by 0x1DA4D4: ft_mem_qrealloc (ftutil.c:145) ==98218== by 0x1DA3E1: ft_mem_realloc (ftutil.c:101) ==98218== by 0x212B48: ft_raster1_render (ftrend1.c:199) ==98218== by 0x1D5A81: FT_Render_Glyph_Internal (ftobjs.c:3812) ==98218== by 0x1D5B32: FT_Render_Glyph (ftobjs.c:3852) ==98218== by 0x1D1ADF: FT_Load_Glyph (ftobjs.c:781) ==98218== ==98218== Invalid read of size 1 ==98218== at 0x115D298: memcpy (mc_replace_strmem.c:482) ==98218== by 0x4C2AC9: bytes_copy_rectangle (gsbitops.c:657) ==98218== by 0x2971BA: cmd_put_bits (gxclbits.c:231) ==98218== by 0x2A94B9: clist_copy_mono (gxclrect.c:632) ==98218== by 0x509D75: clip_copy_mono (gxclip.c:401) ==98218== by 0x5147BE: gx_dc_pure_fill_masked (gxdcolor.c:444) ==98218== by 0x554E21: gx_default_fill_mask (gdevdbit.c:332) ==98218== by 0x2AE6B0: clist_fill_mask (gxclimag.c:102) ==98218== by 0x5395A1: gx_image_fill_masked (gximask.c:105) ==98218== by 0x1BFDB6: fapi_finish_render_aux (zfapi.c:1662) ==98218== by 0x1BFF67: fapi_finish_render (zfapi.c:1691) ==98218== by 0x1C27E2: FAPI_do_char (zfapi.c:2328) ==98218== Address 0x1cbd1ca is 0 bytes after a block of size 5,354 alloc'd ==98218== at 0x1159516: malloc (vg_replace_malloc.c:193) ==98218== by 0x4E79AA: gs_heap_alloc_bytes (gsmalloc.c:182) ==98218== by 0x4E7BAC: gs_heap_alloc_byte_array (gsmalloc.c:241) ==98218== by 0x1C3244: FF_alloc (fapi_ft.c:97) ==98218== by 0x1DA37A: ft_mem_qalloc (ftutil.c:76) ==98218== by 0x1DA314: ft_mem_alloc (ftutil.c:55) ==98218== by 0x1DA4D4: ft_mem_qrealloc (ftutil.c:145) ==98218== by 0x1DA3E1: ft_mem_realloc (ftutil.c:101) ==98218== by 0x212B48: ft_raster1_render (ftrend1.c:199) ==98218== by 0x1D5A81: FT_Render_Glyph_Internal (ftobjs.c:3812) ==98218== by 0x1D5B32: FT_Render_Glyph (ftobjs.c:3852) ==98218== by 0x1D1ADF: FT_Load_Glyph (ftobjs.c:781)
Resolved with revision: 11362.
While working on a different bug, I had to look at the patch in revision 11362 <http://svn.ghostscript.com/viewvc?view=rev&revision=11362>. I may be wrong (in which case I apologize in advance for the noise) but that patch seems incorrect. If I understood correctly, the patch tries to clip the right part of the source bitmap if the passed width is too large. The problem is that ‘src’ may very well point somewhere in the middle of a scanline, so there are less than ‘src_raster’ bytes left for reading in the current scanline. For the last scanline, the code still reads past the end of the bitmap. The ‘bytes_copy_rectangle()’ does not have sufficient information to do this clipping, any clipping has to be done by its callers. I suggest to revert the patch, and verify ‘bytes_copy_rectangle()’s callers.
> > The ‘bytes_copy_rectangle()’ does not have sufficient information to do > this clipping, any clipping has to be done by its callers. I suggest to > revert the patch, and verify ‘bytes_copy_rectangle()’s callers. Agreed, the bug is reopened, thanks SaGS
I had a look at this. Since this is in the clist code, I'll take it. I agree with SaGS that the 11362 patch is not the correct fix, but I also would like Chris and SaGS to review my analysis... I think the problem is that when we are calling bytes_copy_rectangle with short_raster that is 'full_raster', but the 'short_size' is not large enough for the (height * full_raster) number of bytes. This is because the 'short_size' calculation from the clist_bitmap_bytes call that sets 'short_raster' may be expecting that the last line will be truncated: cmd_put_bits calls: uint short_size = clist_bitmap_bytes(width_bits, height, compression_mask & ~cmd_mask_compress_any, &short_raster, &full_raster); ----- clist_bitmap_bytes sets: uint full_raster = *raster = bitmap_raster(width_bits); uint short_raster = (width_bits + 7) >> 3; ----- the clist_bitmap_bytes _may_ take the path: else *width_bytes = full_raster, width_bytes_last = short_raster; return (height == 0 ? 0 : *width_bytes * (height - 1) + width_bytes_last); ----- This will set short_raster to full_raster, but the short_size will expect the smaller width_bytes_last to truncate the last line's padding. When cmd_put_bits calls: bytes_copy_rectangle(dp + op_size, short_raster, data, raster, short_raster, height); This will write short_raster == full_raster for the last line instead of what the clist_bitmap_bytes calculated as 'width_bytes_last'. ---------- For a maximum savings for 3 bytes in an uncommon case, this seems rather pointless, but this could be fixed in cmd_put_bits (but not in the bytes_copy_rectangle as SaGS points out, and as r11532 attempts). ---------- Since all of the valgrind issues were from 'cmd_put_bits' this is the first place to fix, but I will look at all of the other clients of bytes_copy_rectangle to see if any of them have problems and come up with a patch that fixes these (and reverts r11352).
I revisited this last night, after SaGS comment and (now I understand things a little better!) I take the point. Ray, if you want to review and revise how this stuff works at the clist level, I'm fine with that, but the "core" of the problem is that the clist code, and possibly elsewhere, quite reasonably only expect bitmaps originating from within Ghostscript itself. Now, however, it can receive bitmap data from FAPI which originates in the font scaler/renderer library which happens to be in use at the time (by default, Freetype). My concern is that we've seen the clist related problem, but there may be other places that make the same assumption, and I would rather not try to track them all down! The problem here is that Freetype uses a different scanline alignment to Ghostscript. Thus I suggest that the proper solution for *this* problem (regardless of other improvements to be made elsewhere) is to have FAPI ensure that the bitmap is correctly formatted for Ghostscript before calling GS operations on it. I wrote the code to do this last night, and I have a longer term solution for the default Freetype implemenation (which will provide other benefits if it goes ahead) but I need to discuss it with Werner as it means changes to Freetype. Unless there are objections, I'll commit the FAPI changes.
(In reply to comment #5) > > Unless there are objections, I'll commit the FAPI changes. Done with r11542 - also reverts the previous change. The FAPI layer will now detect an unsuitably aligned bitmap, and create a new, GS compatible one before passing it anywhere else in Ghostscript.
*** Bug 691451 has been marked as a duplicate of this bug. ***