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; }