Skip to content

W
wine-cw
Commit 72c0c88f authored by Jinoh Kang's avatar Jinoh Kang Committed by Alexandre Julliard
Browse files
Options

gdiplus: Replace GpImage's busy flag with SRWLOCK.

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.
parent 7abca974
Hide whitespace changes
Inline Side-by-side
Showing with 36 additions and 44 deletions
dlls/gdiplus/gdiplus_private.h
View file @ 72c0c88f
...@@ -371,7 +371,7 @@ struct GpImage{ ...@@ -371,7 +371,7 @@ struct GpImage{
UINT frame_count, current_frame; UINT frame_count, current_frame;
ColorPalette *palette; ColorPalette *palette;
REAL xres, yres; REAL xres, yres;
LONG busy; SRWLOCK lock;
}; };
#define EmfPlusObjectTableSize 64 #define EmfPlusObjectTableSize 64
...@@ -613,17 +613,14 @@ GpStatus gdip_format_string(HDC hdc, ...@@ -613,17 +613,14 @@ GpStatus gdip_format_string(HDC hdc,
void get_log_fontW(const GpFont *, GpGraphics *, LOGFONTW *) DECLSPEC_HIDDEN; 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; return TryAcquireSRWLockExclusive(&image->lock);
owner_tid = InterlockedCompareExchange(&image->busy, tid, 0);
*unlock = !owner_tid;
return !owner_tid || owner_tid==tid;
} }
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) static inline void set_rect(GpRectF *rect, REAL x, REAL y, REAL width, REAL height)
......
dlls/gdiplus/image.c
View file @ 72c0c88f
...@@ -1086,20 +1086,19 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1086,20 +1086,19 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
INT bitspp = PIXELFORMATBPP(format); INT bitspp = PIXELFORMATBPP(format);
GpRect act_rect; /* actual rect to be used */ GpRect act_rect; /* actual rect to be used */
GpStatus stat; GpStatus stat;
BOOL unlock;
TRACE("%p %p %d 0x%x %p\n", bitmap, rect, flags, format, lockeddata); TRACE("%p %p %d 0x%x %p\n", bitmap, rect, flags, format, lockeddata);
if(!lockeddata || !bitmap) if(!lockeddata || !bitmap)
return InvalidParameter; return InvalidParameter;
if(!image_lock(&bitmap->image, &unlock)) if(!image_lock(&bitmap->image))
return ObjectBusy; return ObjectBusy;
if(rect){ if(rect){
if(rect->X < 0 || rect->Y < 0 || (rect->X + rect->Width > bitmap->width) || if(rect->X < 0 || rect->Y < 0 || (rect->X + rect->Width > bitmap->width) ||
(rect->Y + rect->Height > bitmap->height) || !flags) (rect->Y + rect->Height > bitmap->height) || !flags)
{ {
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return InvalidParameter; return InvalidParameter;
} }
...@@ -1114,7 +1113,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1114,7 +1113,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
if(bitmap->lockmode) if(bitmap->lockmode)
{ {
WARN("bitmap is already locked and cannot be locked again\n"); WARN("bitmap is already locked and cannot be locked again\n");
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return WrongState; return WrongState;
} }
...@@ -1131,7 +1130,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1131,7 +1130,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
bitmap->lockmode = flags | ImageLockModeRead; bitmap->lockmode = flags | ImageLockModeRead;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return Ok; return Ok;
} }
...@@ -1142,7 +1141,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1142,7 +1141,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
if (stat == NotImplemented) if (stat == NotImplemented)
{ {
FIXME("cannot read bitmap from %x to %x\n", bitmap->format, format); FIXME("cannot read bitmap from %x to %x\n", bitmap->format, format);
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return NotImplemented; return NotImplemented;
} }
} }
...@@ -1155,7 +1154,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1155,7 +1154,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
if (stat == NotImplemented) if (stat == NotImplemented)
{ {
FIXME("cannot write bitmap from %x to %x\n", format, bitmap->format); FIXME("cannot write bitmap from %x to %x\n", format, bitmap->format);
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return NotImplemented; return NotImplemented;
} }
} }
...@@ -1173,7 +1172,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1173,7 +1172,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
if (!bitmap->bitmapbits) if (!bitmap->bitmapbits)
{ {
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return OutOfMemory; return OutOfMemory;
} }
...@@ -1200,7 +1199,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1200,7 +1199,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
{ {
heap_free(bitmap->bitmapbits); heap_free(bitmap->bitmapbits);
bitmap->bitmapbits = NULL; bitmap->bitmapbits = NULL;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return stat; return stat;
} }
} }
...@@ -1209,7 +1208,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect, ...@@ -1209,7 +1208,7 @@ GpStatus WINGDIPAPI GdipBitmapLockBits(GpBitmap* bitmap, GDIPCONST GpRect* rect,
bitmap->lockx = act_rect.X; bitmap->lockx = act_rect.X;
bitmap->locky = act_rect.Y; bitmap->locky = act_rect.Y;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return Ok; return Ok;
} }
...@@ -1231,18 +1230,17 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, ...@@ -1231,18 +1230,17 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap,
{ {
GpStatus stat; GpStatus stat;
static BOOL fixme = FALSE; static BOOL fixme = FALSE;
BOOL unlock;
TRACE("(%p,%p)\n", bitmap, lockeddata); TRACE("(%p,%p)\n", bitmap, lockeddata);
if(!bitmap || !lockeddata) if(!bitmap || !lockeddata)
return InvalidParameter; return InvalidParameter;
if(!image_lock(&bitmap->image, &unlock)) if(!image_lock(&bitmap->image))
return ObjectBusy; return ObjectBusy;
if(!bitmap->lockmode) if(!bitmap->lockmode)
{ {
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return WrongState; return WrongState;
} }
...@@ -1250,7 +1248,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, ...@@ -1250,7 +1248,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap,
bitmap->lockmode = 0; bitmap->lockmode = 0;
heap_free(bitmap->bitmapbits); heap_free(bitmap->bitmapbits);
bitmap->bitmapbits = NULL; bitmap->bitmapbits = NULL;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return Ok; return Ok;
} }
...@@ -1258,7 +1256,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, ...@@ -1258,7 +1256,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap,
{ {
/* we passed a direct reference; no need to do anything */ /* we passed a direct reference; no need to do anything */
bitmap->lockmode = 0; bitmap->lockmode = 0;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return Ok; return Ok;
} }
...@@ -1284,7 +1282,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap, ...@@ -1284,7 +1282,7 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* bitmap,
bitmap->bitmapbits = NULL; bitmap->bitmapbits = NULL;
bitmap->lockmode = 0; bitmap->lockmode = 0;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return stat; return stat;
} }
...@@ -1517,12 +1515,11 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, ...@@ -1517,12 +1515,11 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap,
UINT width, height; UINT width, height;
BITMAPINFOHEADER bih; BITMAPINFOHEADER bih;
LPBYTE bits; LPBYTE bits;
BOOL unlock;
TRACE("(%p,%p,%lx)\n", bitmap, hbmReturn, background); TRACE("(%p,%p,%lx)\n", bitmap, hbmReturn, background);
if (!bitmap || !hbmReturn) return InvalidParameter; if (!bitmap || !hbmReturn) return InvalidParameter;
if (!image_lock(&bitmap->image, &unlock)) return ObjectBusy; if (!image_lock(&bitmap->image)) return ObjectBusy;
GdipGetImageWidth(&bitmap->image, &width); GdipGetImageWidth(&bitmap->image, &width);
GdipGetImageHeight(&bitmap->image, &height); GdipGetImageHeight(&bitmap->image, &height);
...@@ -1542,7 +1539,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, ...@@ -1542,7 +1539,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap,
result = CreateDIBSection(0, (BITMAPINFO*)&bih, DIB_RGB_COLORS, (void**)&bits, NULL, 0); result = CreateDIBSection(0, (BITMAPINFO*)&bih, DIB_RGB_COLORS, (void**)&bits, NULL, 0);
if (!result) if (!result)
{ {
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return GenericError; return GenericError;
} }
...@@ -1552,7 +1549,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, ...@@ -1552,7 +1549,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap,
if (stat != Ok) if (stat != Ok)
{ {
DeleteObject(result); DeleteObject(result);
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return stat; return stat;
} }
...@@ -1568,7 +1565,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap, ...@@ -1568,7 +1565,7 @@ GpStatus WINGDIPAPI GdipCreateHBITMAPFromBitmap(GpBitmap* bitmap,
} }
*hbmReturn = result; *hbmReturn = result;
image_unlock(&bitmap->image, unlock); image_unlock(&bitmap->image);
return Ok; return Ok;
} }
...@@ -3822,8 +3819,8 @@ static GpStatus select_frame_wic(GpImage *image, UINT active_frame) ...@@ -3822,8 +3819,8 @@ static GpStatus select_frame_wic(GpImage *image, UINT active_frame)
new_image->encoder = image->encoder; new_image->encoder = image->encoder;
image->encoder = NULL; image->encoder = NULL;
free_image_data(image); free_image_data(image);
memcpy(image, new_image, FIELD_OFFSET(GpImage, busy)); memcpy(image, new_image, FIELD_OFFSET(GpImage, lock));
body_offset = RTL_SIZEOF_THROUGH_FIELD(GpImage, busy); body_offset = RTL_SIZEOF_THROUGH_FIELD(GpImage, lock);
memcpy((char *)image + body_offset, (char *)new_image + body_offset, obj_size - body_offset); memcpy((char *)image + body_offset, (char *)new_image + body_offset, obj_size - body_offset);
new_image->type = ~0; new_image->type = ~0;
heap_free(new_image); heap_free(new_image);
...@@ -4361,39 +4358,38 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d ...@@ -4361,39 +4358,38 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d
{ {
GpStatus stat; GpStatus stat;
const struct image_codec *codec = NULL; const struct image_codec *codec = NULL;
BOOL unlock;
TRACE("(%p,%s,%u)\n", image, debugstr_guid(dimensionID), frame); TRACE("(%p,%s,%u)\n", image, debugstr_guid(dimensionID), frame);
if (!image || !dimensionID) if (!image || !dimensionID)
return InvalidParameter; return InvalidParameter;
if(!image_lock(image, &unlock)) if(!image_lock(image))
return ObjectBusy; return ObjectBusy;
if (frame >= image->frame_count) if (frame >= image->frame_count)
{ {
WARN("requested frame %u, but image has only %u\n", 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; return InvalidParameter;
} }
if (image->type != ImageTypeBitmap && image->type != ImageTypeMetafile) if (image->type != ImageTypeBitmap && image->type != ImageTypeMetafile)
{ {
WARN("invalid image type %d\n", image->type); WARN("invalid image type %d\n", image->type);
image_unlock(image, unlock); image_unlock(image);
return InvalidParameter; return InvalidParameter;
} }
if (image->current_frame == frame) if (image->current_frame == frame)
{ {
image_unlock(image, unlock); image_unlock(image);
return Ok; return Ok;
} }
if (!image->decoder) if (!image->decoder)
{ {
TRACE("image doesn't have an associated decoder\n"); TRACE("image doesn't have an associated decoder\n");
image_unlock(image, unlock); image_unlock(image);
return Ok; return Ok;
} }
...@@ -4402,12 +4398,12 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d ...@@ -4402,12 +4398,12 @@ GpStatus WINGDIPAPI GdipImageSelectActiveFrame(GpImage *image, GDIPCONST GUID *d
if (stat != Ok) if (stat != Ok)
{ {
WARN("can't find decoder info\n"); WARN("can't find decoder info\n");
image_unlock(image, unlock); image_unlock(image);
return stat; return stat;
} }
stat = codec->select_func(image, frame); stat = codec->select_func(image, frame);
image_unlock(image, unlock); image_unlock(image);
return stat; return stat;
} }
...@@ -5542,13 +5538,12 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) ...@@ -5542,13 +5538,12 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type)
UINT x, y, width, height; UINT x, y, width, height;
BitmapData dst_lock; BitmapData dst_lock;
GpStatus stat; GpStatus stat;
BOOL unlock;
TRACE("(%p, %u)\n", image, type); TRACE("(%p, %u)\n", image, type);
if (!image) if (!image)
return InvalidParameter; return InvalidParameter;
if (!image_lock(image, &unlock)) if (!image_lock(image))
return ObjectBusy; return ObjectBusy;
rotate_90 = type&1; rotate_90 = type&1;
...@@ -5558,7 +5553,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) ...@@ -5558,7 +5553,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type)
if (image->type != ImageTypeBitmap) if (image->type != ImageTypeBitmap)
{ {
FIXME("Not implemented for type %i\n", image->type); FIXME("Not implemented for type %i\n", image->type);
image_unlock(image, unlock); image_unlock(image);
return NotImplemented; return NotImplemented;
} }
...@@ -5568,7 +5563,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) ...@@ -5568,7 +5563,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type)
if (bpp < 8) if (bpp < 8)
{ {
FIXME("Not implemented for %i bit images\n", bpp); FIXME("Not implemented for %i bit images\n", bpp);
image_unlock(image, unlock); image_unlock(image);
return NotImplemented; return NotImplemented;
} }
...@@ -5638,7 +5633,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type) ...@@ -5638,7 +5633,7 @@ GpStatus WINGDIPAPI GdipImageRotateFlip(GpImage *image, RotateFlipType type)
else GdipDisposeImage(&new_bitmap->image); else GdipDisposeImage(&new_bitmap->image);
} }
image_unlock(image, unlock); image_unlock(image);
return stat; return stat;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment