This fixes data race in ARM/ARM64 platforms, and prevents potential memory access reordering by the compiler.
-- v3: gdiplus: Replace GpImage's busy flag with SRWLOCK. gdiplus: Avoid copying GpImage's busy flag in select_frame_wic(). gdiplus: Avoid recursively locking image in GdipImageRotateFlip.
From: Jinoh Kang jinoh.kang.kr@gmail.com
GdipImageRotateFlip() calls GdipBitmapLockBits() while holding the image lock, resulting in a recursive lock.
Since GdipImageRotateFlip() uses GdipBitmapLockBits() only to obtain Scan0 and Stride, replace them with equivalent fields from GpBitmap itself. --- dlls/gdiplus/image.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index b899619e1d3..93d2a366e82 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -5530,7 +5530,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) int src_x_offset, src_y_offset; LPBYTE src_origin; UINT x, y, width, height; - BitmapData src_lock, dst_lock; + BitmapData dst_lock; GpStatus stat; BOOL unlock;
@@ -5577,14 +5577,6 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type)
stat = GdipCreateBitmapFromScan0(width, height, 0, bitmap->format, NULL, &new_bitmap);
- if (stat != Ok) - { - image_unlock(image, unlock); - return stat; - } - - stat = GdipBitmapLockBits(bitmap, NULL, ImageLockModeRead, bitmap->format, &src_lock); - if (stat == Ok) { stat = GdipBitmapLockBits(new_bitmap, NULL, ImageLockModeWrite, bitmap->format, &dst_lock); @@ -5594,14 +5586,14 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) LPBYTE src_row, src_pixel; LPBYTE dst_row, dst_pixel;
- src_origin = src_lock.Scan0; + src_origin = bitmap->bits; if (flip_x) src_origin += bytesperpixel * (bitmap->width - 1); - if (flip_y) src_origin += src_lock.Stride * (bitmap->height - 1); + if (flip_y) src_origin += bitmap->stride * (bitmap->height - 1);
if (rotate_90) { - if (flip_y) src_x_offset = -src_lock.Stride; - else src_x_offset = src_lock.Stride; + if (flip_y) src_x_offset = -bitmap->stride; + else src_x_offset = bitmap->stride; if (flip_x) src_y_offset = -bytesperpixel; else src_y_offset = bytesperpixel; } @@ -5609,8 +5601,8 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) { if (flip_x) src_x_offset = -bytesperpixel; else src_x_offset = bytesperpixel; - if (flip_y) src_y_offset = -src_lock.Stride; - else src_y_offset = src_lock.Stride; + if (flip_y) src_y_offset = -bitmap->stride; + else src_y_offset = bitmap->stride; }
src_row = src_origin; @@ -5631,16 +5623,11 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) }
GdipBitmapUnlockBits(new_bitmap, &dst_lock); + move_bitmap(bitmap, new_bitmap, FALSE); } - - GdipBitmapUnlockBits(bitmap, &src_lock); + else GdipDisposeImage(&new_bitmap->image); }
- if (stat == Ok) - move_bitmap(bitmap, new_bitmap, FALSE); - else - GdipDisposeImage(&new_bitmap->image); - image_unlock(image, unlock); return stat; }
From: Jinoh Kang jinoh.kang.kr@gmail.com
The 'busy' field in GpImage is used as an atomic variable. The C11 standard (§5.1.2.4, paragraph 25) states that two conflicting actions to a memory location shall be both atomic operations, or otherwise properly synchronized; otherwise, it constitutes a data race.
However, select_frame_wic() performs a non-atomic access to the 'busy' field on a GpImage that is potentially accessible by other threads. This happens when select_frame_wic() copies new_image to the old image object. Although it does attempt to preserve the value of the 'busy' field by setting new_image->busy = image->busy first, thereby effectively assigning an identical value to the field, it is unclear that this does not actually constitute a theoretical, if not practical, data race. This also prevents replacing the busy flag with a mutex or other synchronization primitives.
Therefore, skip the 'busy' field when copying fields from the new image to the original image object. --- dlls/gdiplus/image.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 93d2a366e82..629599ddf2c 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -3799,6 +3799,7 @@ static GpStatus decode_image_wic(IStream *stream, REFGUID container,
static GpStatus select_frame_wic(GpImage *image, UINT active_frame) { + size_t obj_size, body_offset; GpImage *new_image; GpStatus status;
@@ -3806,15 +3807,24 @@ static GpStatus select_frame_wic(GpImage *image, UINT active_frame) if(status != Ok) return status;
- new_image->busy = image->busy; + if (image->type == ImageTypeBitmap) + obj_size = sizeof(GpBitmap); + else if (image->type == ImageTypeMetafile) + obj_size = sizeof(GpMetafile); + else + { + ERR("unknown image type: %d\n", image->type); + GdipDisposeImage(new_image); + return GenericError; + } + memcpy(&new_image->format, &image->format, sizeof(GUID)); new_image->encoder = image->encoder; image->encoder = NULL; free_image_data(image); - if (image->type == ImageTypeBitmap) - *(GpBitmap *)image = *(GpBitmap *)new_image; - else if (image->type == ImageTypeMetafile) - *(GpMetafile *)image = *(GpMetafile *)new_image; + memcpy(image, new_image, FIELD_OFFSET(GpImage, busy)); + body_offset = RTL_SIZEOF_THROUGH_FIELD(GpImage, busy); + memcpy((char *)image + body_offset, (char *)new_image + body_offset, obj_size - body_offset); new_image->type = ~0; heap_free(new_image); return Ok;
From: Jinoh Kang jinoh.kang.kr@gmail.com
Today, the image_unlock() helper function has a data race due to non-atomic write to GpImage's 'busy' flag which is accessible by other threads. Also, it lacks a release fence, which means that other threads can observe the unlocked (busy = 0) state too early when the current thread unlocks the image; specifically, the write to the 'busy' field of the GpImage can be reordered before the last read/write to any other fields of the same GpImage.
Fix this by replacing the 'busy' field of GpImage with SRWLOCK. --- dlls/gdiplus/gdiplus_private.h | 13 +++---- dlls/gdiplus/image.c | 67 ++++++++++++++++------------------ 2 files changed, 36 insertions(+), 44 deletions(-)
diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h index 7ae8cc564c9..03bfa5513cb 100644 --- a/dlls/gdiplus/gdiplus_private.h +++ b/dlls/gdiplus/gdiplus_private.h @@ -371,7 +371,7 @@ struct GpImage{ UINT frame_count, current_frame; ColorPalette *palette; REAL xres, yres; - LONG busy; + SRWLOCK lock; };
#define EmfPlusObjectTableSize 64 @@ -613,17 +613,14 @@ GpStatus gdip_format_string(HDC hdc,
void get_log_fontW(const GpFont *, GpGraphics *, LOGFONTW *) DECLSPEC_HIDDEN;
-static inline BOOL image_lock(GpImage *image, BOOL *unlock) +static inline BOOL image_lock(GpImage *image) { - LONG tid = GetCurrentThreadId(), owner_tid; - owner_tid = InterlockedCompareExchange(&image->busy, tid, 0); - *unlock = !owner_tid; - return !owner_tid || owner_tid==tid; + return TryAcquireSRWLockExclusive(&image->lock); }
-static inline void image_unlock(GpImage *image, BOOL unlock) +static inline void image_unlock(GpImage *image) { - if (unlock) image->busy = 0; + ReleaseSRWLockExclusive(&image->lock); }
static inline void set_rect(GpRectF *rect, REAL x, REAL y, REAL width, REAL height) diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 629599ddf2c..101e7ae1c93 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -1086,20 +1086,19 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, INT bitspp = PIXELFORMATBPP(format); GpRect act_rect; /* actual rect to be used */ GpStatus stat; - BOOL unlock;
TRACE("%p %p %d 0x%x %p\n", bitmap, rect, flags, format, lockeddata);
if(!lockeddata || !bitmap) return InvalidParameter; - if(!image_lock(&bitmap->image, &unlock)) + if(!image_lock(&bitmap->image)) return ObjectBusy;
if(rect){ if(rect->X < 0 || rect->Y < 0 || (rect->X + rect->Width > bitmap->width) || (rect->Y + rect->Height > bitmap->height) || !flags) { - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return InvalidParameter; }
@@ -1114,7 +1113,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, if(bitmap->lockmode) { WARN("bitmap is already locked and cannot be locked again\n"); - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return WrongState; }
@@ -1131,7 +1130,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
bitmap->lockmode = flags | ImageLockModeRead;
- image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return Ok; }
@@ -1142,7 +1141,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, if (stat == NotImplemented) { FIXME("cannot read bitmap from %x to %x\n", bitmap->format, format); - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return NotImplemented; } } @@ -1155,7 +1154,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, if (stat == NotImplemented) { FIXME("cannot write bitmap from %x to %x\n", format, bitmap->format); - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return NotImplemented; } } @@ -1173,7 +1172,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
if (!bitmap->bitmapbits) { - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return OutOfMemory; }
@@ -1200,7 +1199,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, { heap_free(bitmap->bitmapbits); bitmap->bitmapbits = NULL; - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return stat; } } @@ -1209,7 +1208,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, bitmap->lockx = act_rect.X; bitmap->locky = act_rect.Y;
- image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return Ok; }
@@ -1231,18 +1230,17 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, { GpStatus stat; static BOOL fixme = FALSE; - BOOL unlock;
TRACE("(%p,%p)\n", bitmap, lockeddata);
if(!bitmap || !lockeddata) return InvalidParameter; - if(!image_lock(&bitmap->image, &unlock)) + if(!image_lock(&bitmap->image)) return ObjectBusy;
if(!bitmap->lockmode) { - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return WrongState; }
@@ -1250,7 +1248,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, bitmap->lockmode = 0; heap_free(bitmap->bitmapbits); bitmap->bitmapbits = NULL; - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return Ok; }
@@ -1258,7 +1256,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, { /* we passed a direct reference; no need to do anything */ bitmap->lockmode = 0; - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return Ok; }
@@ -1284,7 +1282,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, bitmap->bitmapbits = NULL; bitmap->lockmode = 0;
- image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return stat; }
@@ -1517,12 +1515,11 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, UINT width, height; BITMAPINFOHEADER bih; LPBYTE bits; - BOOL unlock;
TRACE("(%p,%p,%lx)\n", bitmap, hbmReturn, background);
if (!bitmap || !hbmReturn) return InvalidParameter; - if (!image_lock(&bitmap->image, &unlock)) return ObjectBusy; + if (!image_lock(&bitmap->image)) return ObjectBusy;
GdipGetImageWidth(&bitmap->image, &width); GdipGetImageHeight(&bitmap->image, &height); @@ -1542,7 +1539,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, result = CreateDIBSection(0, (BITMAPINFO*)&bih, DIB_RGB_COLORS, (void**)&bits, NULL, 0); if (!result) { - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return GenericError; }
@@ -1552,7 +1549,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, if (stat != Ok) { DeleteObject(result); - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return stat; }
@@ -1568,7 +1565,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, }
*hbmReturn = result; - image_unlock(&bitmap->image, unlock); + image_unlock(&bitmap->image); return Ok; }
@@ -3822,8 +3819,8 @@ static GpStatus select_frame_wic(GpImage *image, UINT active_frame) new_image->encoder = image->encoder; image->encoder = NULL; free_image_data(image); - memcpy(image, new_image, FIELD_OFFSET(GpImage, busy)); - body_offset = RTL_SIZEOF_THROUGH_FIELD(GpImage, busy); + memcpy(image, new_image, FIELD_OFFSET(GpImage, lock)); + body_offset = RTL_SIZEOF_THROUGH_FIELD(GpImage, lock); memcpy((char *)image + body_offset, (char *)new_image + body_offset, obj_size - body_offset); new_image->type = ~0; heap_free(new_image); @@ -4361,39 +4358,38 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d { GpStatus stat; const struct image_codec *codec = NULL; - BOOL unlock;
TRACE("(%p,%s,%u)\n", image, debugstr_guid(dimensionID), frame);
if (!image || !dimensionID) return InvalidParameter; - if(!image_lock(image, &unlock)) + if(!image_lock(image)) return ObjectBusy;
if (frame >= image->frame_count) { WARN("requested frame %u, but image has only %u\n", frame, image->frame_count); - image_unlock(image, unlock); + image_unlock(image); return InvalidParameter; }
if (image->type != ImageTypeBitmap && image->type != ImageTypeMetafile) { WARN("invalid image type %d\n", image->type); - image_unlock(image, unlock); + image_unlock(image); return InvalidParameter; }
if (image->current_frame == frame) { - image_unlock(image, unlock); + image_unlock(image); return Ok; }
if (!image->decoder) { TRACE("image doesn't have an associated decoder\n"); - image_unlock(image, unlock); + image_unlock(image); return Ok; }
@@ -4402,12 +4398,12 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d if (stat != Ok) { WARN("can't find decoder info\n"); - image_unlock(image, unlock); + image_unlock(image); return stat; }
stat = codec->select_func(image, frame); - image_unlock(image, unlock); + image_unlock(image); return stat; }
@@ -5542,13 +5538,12 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) UINT x, y, width, height; BitmapData dst_lock; GpStatus stat; - BOOL unlock;
TRACE("(%p, %u)\n", image, type);
if (!image) return InvalidParameter; - if (!image_lock(image, &unlock)) + if (!image_lock(image)) return ObjectBusy;
rotate_90 = type&1; @@ -5558,7 +5553,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) if (image->type != ImageTypeBitmap) { FIXME("Not implemented for type %i\n", image->type); - image_unlock(image, unlock); + image_unlock(image); return NotImplemented; }
@@ -5568,7 +5563,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) if (bpp < 8) { FIXME("Not implemented for %i bit images\n", bpp); - image_unlock(image, unlock); + image_unlock(image); return NotImplemented; }
@@ -5638,7 +5633,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) else GdipDisposeImage(&new_bitmap->image); }
- image_unlock(image, unlock); + image_unlock(image); return stat; }
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125300
Your paranoid android.
=== debian11 (build log) ===
Task: Could not create the win32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the win32 report
These memory barriers do nothing on x86, and don't even exist (compile to no code at all),
Not emitting any machine code does not imply that it has no effect on code generation at all. In fact, the fences act as compile-time memory barriers, preventing the code optimizer from reordering memory accesses.
Patch 1, `gdiplus: Avoid recursively locking image in GdipImageRotateFlip.`, changes the behavior of `GdipImageRotateFlip`, as it will no longer check whether the bitmap is already locked. We don't have tests for this, so I'm not sure which way is correct.
On Sun Oct 23 15:41:11 2022 +0000, Esme Povirk wrote:
Patch 1, `gdiplus: Avoid recursively locking image in GdipImageRotateFlip.`, changes the behavior of `GdipImageRotateFlip`, as it will no longer check whether the bitmap is already locked. We don't have tests for this, so I'm not sure which way is correct.
According to https://testbot.winehq.org/JobDetails.pl?Key=125324, GDI+ acts weird when the bitmap is rotate-flipped while being locked.
Some observations:
1. GdipImageRotateFlip succeeds after GdipBitmapLockBits. 2. After rotate-flip, the bitmap is still in locked state. 3. GdipBitmapUnlockBits fails with error 7 (Win32Error) after GdipImageRotateFlip. 4. Subsequent attempts to access the pixels or lock bits succeeds, but the Scan0 is different from the initial BitmapData.
Normally I'd add these to tests, but I'm afraid I'm actually running into undefined behaviour. Perhaps test in a child process?
On Sun Oct 23 15:41:11 2022 +0000, Jinoh Kang wrote:
According to https://testbot.winehq.org/JobDetails.pl?Key=125324, GDI+ acts weird when the bitmap is rotate-flipped while being locked. Some observations:
- GdipImageRotateFlip succeeds after GdipBitmapLockBits.
- After rotate-flip, the bitmap is still in locked state.
- GdipBitmapUnlockBits fails with error 7 (Win32Error) after GdipImageRotateFlip.
- Subsequent attempts to access the pixels or lock bits succeeds, but
the Scan0 is different from the initial BitmapData. Normally I'd add these to tests, but I'm afraid I'm actually running into undefined behaviour. Perhaps test in a child process?
I think that behavior is strange enough that we shouldn't worry about it, at least not until we see a real app doing this. Thank you for checking.
This merge request was approved by Esme Povirk.
-
Esme Povirk (＠madewokherd) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Marvin