Fix uiImage on mac #402

mischnic · 2018-07-19T21:21:16Z

Fix uiImageAppend on macOS (too much memory was allocated and the copying wasn't correct)
(not sure if the corresponding linux code is correct)

=================================================================
==21689==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000109017da2 at pc 0x0001090b7a82 bp 0x7ffee6c0ce50 sp 0x7ffee6c0ce48
READ of size 1 at 0x000109017da2 thread T0
    #0 0x1090b7a81 in uiImageAppend image.m:48
    #1 0x109005635 in appendImageNamed images.c:196
    #2 0x109014ed6 in makePage16 page16.c:110
    #3 0x10900653d in main main.c:162
    #4 0x7fff7e0aa014 in start (libdyld.dylib:x86_64+0x1014)

0x000109017da2 is located 2 bytes to the right of global variable 'dat0' defined in 'libui-table/test/images.c:4:23' (0x1090179a0) of size 1024
SUMMARY: AddressSanitizer: global-buffer-overflow image.m:48 in uiImageAppend

andlabs · 2018-07-19T23:01:57Z

Half of those I've already fixed and I thought I pushed it but I guess I didn't? Will check again later.

mischnic · 2018-07-20T07:54:08Z

Why is the stride even needed ? Only for future usage ? The macOS code supports only bytePerPixel = 4 because of the data reordering:

libui/darwin/image.m

Lines 48 to 52 in cda991b

    
           sp[0] = bp[2]; 
        
           sp[1] = bp[1]; 
        
           sp[2] = bp[0]; 
        
           sp[3] = bp[3]; 
        
           sp += 4;

and

libui/darwin/image.m

Lines 60 to 61 in cda991b

    
           bitsPerSample:8 
        
           samplesPerPixel:4

Linux uses with CAIRO_FORMAT_ARGB32 and calculates it's own stride, so also 4 bytes:

libui/unix/image.c

Line 45 in cda991b

cstride = cairo_format_stride_for_width(CAIRO_FORMAT_ARGB32, pixelWidth);

Windows also uses GUID_WICPixelFormat32bppRGBA

andlabs · 2018-07-21T21:42:49Z

All right, I actually pushed the fixes this time. Try again. I know there are some things you said that I didn't cover in those fixes, so I'm not sure which still need to be merged in.

Stride is a standard parameter when storing image data in memory, because it's sometimes faster to have extra columns on the right side that are ignored when actually drawing the image, depending on the hardware.

mischnic · 2018-07-21T22:28:20Z

I updated the PR.

Remaining problems:

Running the tester on macOS, I get this when closing the window:

2018-07-22 00:18:36.802 test[72161:1300621] get 0x0
main window 0x6120000346d0
size
in onClosing()
after uiMain()
ASAN:DEADLYSIGNAL
=================================================================
==72161==ERROR: AddressSanitizer: SEGV on unknown address 0x7fff3e000168 (pc 0x7fff7d488174 bp 0x7ffee6555e40 sp 0x7ffee6555e40 T0)
==72161==The signal is caused by a READ memory access.
    #0 0x7fff7d488173 in objc_release (libobjc.A.dylib:x86_64+0x9173)
    #1 0x7fff7d489041 in (anonymous namespace)::AutoreleasePoolPage::pop(void*) (libobjc.A.dylib:x86_64+0xa041)
    #2 0x7fff5618d895 in _CFAutoreleasePoolPop (CoreFoundation:x86_64+0x42895)
    #3 0x7fff582e3b55 in -[NSAutoreleasePool release] (Foundation:x86_64+0x20b55)
    #4 0x109916e33 in uiUninit main.m:144
    #5 0x1096bd58b in main main.c:178
    #6 0x7fff7e0aa014 in start (libdyld.dylib:x86_64+0x1014)

==72161==Register values:
rax = 0x00007fff3e000148  rbx = 0x00006250002b8010  rcx = 0x00006250002b8ca0  rdx = 0x0000000000000204
rdi = 0x00006080000ccda0  rsi = 0x00000001097800e0  rbp = 0x00007ffee6555e40  rsp = 0x00007ffee6555e40
 r8 = 0x000060400016c580   r9 = 0x0000000000000000  r10 = 0xffffffffffffffff  r11 = 0x00000fffffffffff
r12 = 0xa3a3a3a3a3a3a3a3  r13 = 0x0000000000000000  r14 = 0x0000625000006038  r15 = 0x0000625000006000
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (libobjc.A.dylib:x86_64+0x9173) in objc_release
==72161==ABORTING
Abort trap: 6

It doesn't happen with this line commented out:

libui/darwin/image.m

Line 25 in cda991b

[i->i release];

On Linux, this line

libui/unix/image.c

Line 46 in cda991b

    
           buf = (unsigned char *) uiprivAlloc((cstride * pixelHeight * 4) * sizeof (unsigned char), "unsigned char[]");

allocates cstride * pixelHeight * 4 = pixelWidth * 4 * pixelHeight * 4 bytes which seems to be 4 times too many.

msink · 2018-07-26T06:28:53Z

darwin/image.m

@@ -40,18 +40,21 @@ void uiImageAppend(uiImage *i, void *pixels, int pixelWidth, int pixelHeight, in
 	// OS X demands that R and B are in the opposite order from what we expect
 	// we must swizzle :(
 	// LONGTERM test on a big-endian system
-	swizzled = (uint8_t *) uiprivAlloc((pixelStride * pixelHeight * 4) * sizeof (uint8_t), "uint8_t[]");
+	swizzled = (uint8_t *) uiprivAlloc((pixelWidth * pixelHeight * 4) * sizeof (uint8_t), "uint8_t[]");


I think it should be just (pixelStride * pixelHeight).

On linux too, on windows it is already

On Linux, it computes cstride, which is basically pixelWidth * 4.
If I understood andlabs correctly, then the input pixels array can contain unused data at the end of every row. We don't need this data for drawing the image so pixelWidth * 4 should be fine. pixelWidth * 4 is in most cases = pixelStride, but always pixelWidth * 4 < pixelStride.

On Windows, pixelStride is passed to the parameter cbStride: "The number of bytes between successive scanlines in pbBuffer". So the row size of the input data.

msink · 2018-07-26T07:43:26Z

If we want to be cross-platform, stride should always be "row size of the input data", in bytes.

But maybe I'm wrong - as this function is not optimized for speed anyway, copying bytes one-by-one is slow - so different input and internal stride is ok.

For realtime there should be different function - returning pointer to internal framebuffer, on what game engine will draw directly.

mischnic · 2018-07-26T13:39:57Z

If we want to be cross-platform, stride should always be "row size of the input data", in bytes.

It is (the pixelStride parameter).

But maybe I'm wrong - as this function is not optimized for speed anyway, copying bytes one-by-one is slow - so different input and internal stride is ok.
For realtime there should be different function - returning pointer to internal framebuffer, on what game engine will draw directly.

From #2 (comment):

I've now started a slightly different image API based on the fact UI elements that need to render in high-DPI systems use separate image file assets. Right now, this is going to be used in uiTable for image cell parts (don't worry about what this means just yet; just know that multiple parts of different types can make one cell). Not sure if a uiImageView will be provided in alpha 4 or not.

So this is really an uiIcon rather than uiImage.

andlabs · 2018-07-26T16:24:01Z

I've already forgotten what the precise definition of stride is in libui. I'll have to go back and fix that, since it's part of the documentation.

mischnic · 2018-07-29T15:40:15Z

The ASAN crash from: #402 (comment):

2018-07-29 17:37:40.481 test[15512:105095] get 0x0
main window 0x6120000346d0
in onClosing()
after uiMain()
ASAN:DEADLYSIGNAL
=================================================================
==15512==ERROR: AddressSanitizer: SEGV on unknown address 0x7fff020000c8 (pc 0x7fff6c0d3174 bp 0x7ffeeea0fe60 sp 0x7ffeeea0fe60 T0)
==15512==The signal is caused by a READ memory access.
    #0 0x7fff6c0d3173 in objc_release (libobjc.A.dylib:x86_64+0x9173)
    #1 0x7fff6c0d4041 in (anonymous namespace)::AutoreleasePoolPage::pop(void*) (libobjc.A.dylib:x86_64+0xa041)
    #2 0x7fff44dd8895 in _CFAutoreleasePoolPop (CoreFoundation:x86_64+0x42895)
    #3 0x7fff46f2eb55 in -[NSAutoreleasePool release] (Foundation:x86_64+0x20b55)
    #4 0x1012b7e33 in uiUninit main.m:144
    #5 0x10120394b in main main.c:178
    #6 0x7fff6ccf5014 in start (libdyld.dylib:x86_64+0x1014)

==15512==Register values:
rax = 0x00007fff020000a8  rbx = 0x0000625000143010  rcx = 0x0000625000143820  rdx = 0x00000000000001a6
rdi = 0x00006080000485a0  rsi = 0x00000001025510e0  rbp = 0x00007ffeeea0fe60  rsp = 0x00007ffeeea0fe60
 r8 = 0x0000604000069f80   r9 = 0x0000000000000000  r10 = 0xffffffffffffffff  r11 = 0x00000fffffffffff
r12 = 0xa3a3a3a3a3a3a3a3  r13 = 0x0000000000000000  r14 = 0x0000625000006038  r15 = 0x0000625000006000
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (libobjc.A.dylib:x86_64+0x9173) in objc_release
==15512==ABORTING
Abort trap: 6

happens only if at least one other page is included besides page 16.

I think it has something to do with these two release calls:

libui/darwin/image.m

Lines 68 to 73 in cda991b

    
           repsRGB = [repCalibrated bitmapImageRepByRetaggingWithColorSpace:[NSColorSpace sRGBColorSpace]]; 
        
           [repCalibrated release]; 
        
           [i->i addRepresentation:repsRGB]; 
        
           [repsRGB setSize:i->size]; 
        
           [repsRGB release];

The ASAN doesn't trigger if one call is removed. Does bitmapImageRepByRetaggingWithColorSpace copy anything / is there memory that is freed twice?

andlabs · 2018-07-29T16:00:02Z

What are the two calls that can be removed? What you are describing only makes sense if the line that is removed is [repsRGB release]; (which I think is a bug, depending on the docs of bitmapImageRepByRetaggingWithColorSpace:).

mischnic · 2018-07-29T16:11:17Z

What you are describing only makes sense if the line that is removed is [repsRGB release];

Yes, that line. No change if the other one is removed.

(which I think is a bug, depending on the docs of bitmapImageRepByRetaggingWithColorSpace:).

The docs say:

If the original NSBitmapImageRep already uses that color space, it is returned as is.

So if it doesn't allocate anything new in that case, then it shouldn't do it in the other case either?

On here, it says:

bitmapImageRepByRetaggingWithColorSpace:
Changes the colorSpace tag of the receiver.

So: imageRep.colorSpace = ... ? But not really, because printing the color space of repCalibrated and repsRGB after the retagging call show different values - it has create a new instance?

andlabs · 2018-07-29T17:23:50Z

Right, it's a double-free.

andlabs · 2018-07-29T17:27:11Z

Okay, I fixed it in a more correct way than just removing that line. Does asan still complain?

mischnic · 2018-07-29T17:40:30Z

Okay, I fixed it in a more correct way than just removing that line. Does asan still complain?

No 👍

andlabs · 2018-07-29T17:42:16Z

In that case, what's left in this PR?

mischnic · 2018-07-29T17:44:41Z

In that case, what's left in this PR?

That double-free was never part of this PR.
uiImageAppend allocated 4 times too much memory and didn't copy the data correctly.

I think that the same bug also still exists on Linux, this line

libui/unix/image.c

Line 46 in cda991b

    
           buf = (unsigned char *) uiprivAlloc((cstride * pixelHeight * 4) * sizeof (unsigned char), "unsigned char[]");

allocates cstride * pixelHeight * 4 = pixelWidth * 4 * pixelHeight * 4 bytes which seems to be 4 times too many.

andlabs · 2018-08-05T21:48:43Z

I made a decision

// uiImageAppend adds a representation to the uiImage.
// pixels should point to a byte array of non-premultiplied pixels
// stored in [R G B A] order (so ((uint8_t *) pixels)[0] is the R of the
// first pixel and [3] is the A of the first pixel). pixelWidth and
// pixelHeight is the size *in pixels* of the image, and pixelStride is
// the number *of bytes* per row of the pixels array. Therefore,
// pixels itself must be at least pixelStride * pixelHeight bytes long.

If libui doesn't follow this, we'll have to fix it. I'll do that before merging.

@mischnic

…). Also prevents overallocation on some platforms. Thanks to @mischnic and @msink for spotting this. Update #402.

andlabs · 2018-08-05T22:42:27Z

All right, this should be resolved now. Thanks again! What's left in this PR to merge in?

And I forget if I asked already, but: what cmake/make line are you using to build libui with the sanitizers? I'd like to know so I could do so too.

msink · 2018-08-06T05:19:54Z

Is pixelStride needed at all as uiImageAppend parameter, if it is always pixelWidth * 4 ?

mischnic · 2018-08-06T07:09:02Z

What's left in this PR to merge in?

The macOS uiImageAppend byte "swizzling" would only work for byteStride == pixelWidth * 4. Now that should be handled correctly.

what cmake/make line are you using to build libui with the sanitizers?

cd build && cmake .. -DADDRESS_SANITIZER=ON

Handled here:

libui/CMakeLists.txt

Line 129 in cda991b

if(ADDRESS_SANITIZER)

andlabs · 2018-08-06T08:36:46Z

All right, thanks. n and l can be changed back to y and x now; then I can merge this back in. (Also correction: the loop I wrote only works for byteStride being any multiple of 4, not just for pixelWidth * 4. Thanks again, and sorry for the slowness in this PR!)

@msink no, that's just what the minimum value of the pixel stride is. What the stride actually is is a question for the person supplying the image data. Perhaps some image decoding library like libpng sets a custom stride. It's a standard parameter when handling raw image data that every library provides either as an input, output, or both, and libui is no different; the point is that some image manipulations can be made faster if the pixels of each scanline are aligned in a way that seems unusual when thinking about the image as strictly a matrix of pixels.

mischnic · 2018-08-06T09:00:22Z

n and l can be changed back to y and x now; then I can merge this back in.

Done.

msink · 2018-08-06T09:01:30Z

the point is that some image manipulations can be made faster if the pixels of each scanline are aligned in a way that seems unusual when thinking about the image as strictly a matrix of pixels

I still not get - anyway internal stride will/can differ from input parameter of uiImageAppend.
And uiImageAppend supports only RGBA truecolor bitmaps - so why pixelStride needed for input?
If it ever will support something else - additional parameter will be needed.

But of course I do not insist on anything, just curious.

mischnic · 2018-08-06T09:06:00Z

I still not get - anyway internal stride will/can differ from input parameter of uiImageAppend.
And uiImageAppend supports only RGBA truecolor bitmaps - so why pixelStride needed for input?
If it ever will support something else - additional parameter will be needed.

That's what I though at first as well.
The stride isn't for specifying the image color format. Suppose you can decode an image 10 times faster if it is a perfect square. I is your image and X are unused padding bytes.
You set pixelWidth to the number of Is and byteStride based on the number of actual image pixels (I) and padding data (X).

I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X
I I I I I I I I I I I I I I I X X X X X X

msink · 2018-08-06T09:22:21Z

Suppose you can decode an image 10 times faster if it is a perfect square.

On specific hardware - maybe, so it is internal to libui.
But why cross-platform uiImageAppend needs it - I still not get...
Input bitmap is not used anyway - always copied to internal buffer, and input stride used only during copy.

andlabs · 2018-08-06T12:14:54Z

Ideally there should be no copying — on Windows, there is none; we just give the image data directly to WIC! I'm only providing it because every other API I've seen that deals with raw image data provides it, including the low-level ones libui uses, or it explicitly specifies that the stride is always == pixelWidth * bytesPerPixel. I do see your point that libui doesn't seem to need the stride, and now you're starting to make me wonder if I should have something like that one cairo function. But I'll leave that as a TODO.

Anyway, thanks again! Is the address sanitizer cmake stuff another PR? I'll merge that in (with an explanation of why it needs -fno-omit-frame-pointer, a setting I didn't even know was NOT the default!).

mischnic · 2018-08-06T12:18:43Z

Is the address sanitizer cmake stuff another PR? I'll merge that in (with an explanation of -fno-omit-frame-pointer, which I didn't even know was NOT the default!).

It's already in master (added in 92b860c)? It's just not not documented. The frame-pointer option basically turns off an optimization to create more useful stack traces.

libui/CMakeLists.txt

Lines 128 to 129 in cda991b

    
           # TODO document this 
        
           if(ADDRESS_SANITIZER)

andlabs · 2018-08-06T14:47:27Z

Oh. I'm a forgetful dolt sometimes ^^;

mischnic force-pushed the table-image-fix branch from a9068d5 to 463daf4 Compare July 20, 2018 13:55

mischnic force-pushed the table-image-fix branch from 463daf4 to da1d870 Compare July 21, 2018 22:18

mischnic force-pushed the table-image-fix branch from da1d870 to fb694ed Compare July 22, 2018 09:53

msink reviewed Jul 26, 2018

View reviewed changes

andlabs added a commit that referenced this pull request Jul 29, 2018

Fix double-free spotted by @mischnic in #402.

4eaf01f

mischnic force-pushed the table-image-fix branch from 9f401b7 to fb694ed Compare July 29, 2018 17:41

andlabs added a commit that referenced this pull request Aug 5, 2018

Resolved confusion about the terminology of strides in uiImageAppend(…

e0ca00e

…). Also prevents overallocation on some platforms. Thanks to @mischnic and @msink for spotting this. Update #402.

mischnic force-pushed the table-image-fix branch 2 times, most recently from c5c9b81 to 3b13d33 Compare August 6, 2018 07:08

mischnic force-pushed the table-image-fix branch from 3b13d33 to 7793071 Compare August 6, 2018 07:12

Mac uiImageAppend: handle stride correctly

c0742d3

mischnic force-pushed the table-image-fix branch from 7793071 to c0742d3 Compare August 6, 2018 07:28

Rename loop variable

68dade3

andlabs merged commit 976279e into andlabs:table Aug 6, 2018

mischnic deleted the table-image-fix branch August 6, 2018 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix uiImage on mac #402

Fix uiImage on mac #402

mischnic commented Jul 19, 2018 •

edited

andlabs commented Jul 19, 2018

mischnic commented Jul 20, 2018 •

edited

andlabs commented Jul 21, 2018 •

edited

mischnic commented Jul 21, 2018 •

edited

msink Jul 26, 2018 •

edited

mischnic Jul 26, 2018 •

edited

msink commented Jul 26, 2018 •

edited

mischnic commented Jul 26, 2018 •

edited

andlabs commented Jul 26, 2018

mischnic commented Jul 29, 2018 •

edited

andlabs commented Jul 29, 2018

mischnic commented Jul 29, 2018 •

edited

andlabs commented Jul 29, 2018

andlabs commented Jul 29, 2018

mischnic commented Jul 29, 2018

andlabs commented Jul 29, 2018

mischnic commented Jul 29, 2018 •

edited

andlabs commented Aug 5, 2018 •

edited

andlabs commented Aug 5, 2018 •

edited

msink commented Aug 6, 2018 •

edited

mischnic commented Aug 6, 2018

andlabs commented Aug 6, 2018

mischnic commented Aug 6, 2018

msink commented Aug 6, 2018

mischnic commented Aug 6, 2018

msink commented Aug 6, 2018

andlabs commented Aug 6, 2018 •

edited

mischnic commented Aug 6, 2018 •

edited

andlabs commented Aug 6, 2018

Fix uiImage on mac #402

Fix uiImage on mac #402

Conversation

mischnic commented Jul 19, 2018 • edited

andlabs commented Jul 19, 2018

mischnic commented Jul 20, 2018 • edited

andlabs commented Jul 21, 2018 • edited

mischnic commented Jul 21, 2018 • edited

msink Jul 26, 2018 • edited

Choose a reason for hiding this comment

mischnic Jul 26, 2018 • edited

Choose a reason for hiding this comment

msink commented Jul 26, 2018 • edited

mischnic commented Jul 26, 2018 • edited

andlabs commented Jul 26, 2018

mischnic commented Jul 29, 2018 • edited

andlabs commented Jul 29, 2018

mischnic commented Jul 29, 2018 • edited

andlabs commented Jul 29, 2018

andlabs commented Jul 29, 2018

mischnic commented Jul 29, 2018

andlabs commented Jul 29, 2018

mischnic commented Jul 29, 2018 • edited

andlabs commented Aug 5, 2018 • edited

andlabs commented Aug 5, 2018 • edited

msink commented Aug 6, 2018 • edited

mischnic commented Aug 6, 2018

andlabs commented Aug 6, 2018

mischnic commented Aug 6, 2018

msink commented Aug 6, 2018

mischnic commented Aug 6, 2018

msink commented Aug 6, 2018

andlabs commented Aug 6, 2018 • edited

mischnic commented Aug 6, 2018 • edited

andlabs commented Aug 6, 2018

mischnic commented Jul 19, 2018 •

edited

mischnic commented Jul 20, 2018 •

edited

andlabs commented Jul 21, 2018 •

edited

mischnic commented Jul 21, 2018 •

edited

msink Jul 26, 2018 •

edited

mischnic Jul 26, 2018 •

edited

msink commented Jul 26, 2018 •

edited

mischnic commented Jul 26, 2018 •

edited

mischnic commented Jul 29, 2018 •

edited

mischnic commented Jul 29, 2018 •

edited

mischnic commented Jul 29, 2018 •

edited

andlabs commented Aug 5, 2018 •

edited

andlabs commented Aug 5, 2018 •

edited

msink commented Aug 6, 2018 •

edited

andlabs commented Aug 6, 2018 •

edited

mischnic commented Aug 6, 2018 •

edited