Skip to content

Commit 3dcc78c

Browse files
lilithlilith
committed
fix(security): AnimDecoder ctor cleanup, finalizer, overflow guard
- Wrap constructor body in try/catch: on any post-Alloc failure (OptionsInit, AnimDecoderNew, GetInfo, dimension validation), free the native decoder and unpin the input buffer before propagating. Previously a malformed animation that passed New but failed GetInfo leaked both the native handle and the pinned managed buffer — a DoS vector since the caller can feed many such inputs. - Reject canvas dimensions outside (0, WEBP_MAX_DIMENSION] in the ctor so subsequent Width*Height*4 cannot wrap int32. - Widen the Width*Height*4 multiplication in GetNextFrame to long and reject overflow before allocation. Defense-in-depth alongside the ctor cap. - Add ~AnimDecoder() finalizer + Dispose(bool) pattern. Callers who forget Dispose no longer leak the native decoder for the GC lifetime of the object. Finalizer path skips FixDllNotFoundException (which can take locks) and calls WebPAnimDecoderDelete directly.
1 parent 290ff05 commit 3dcc78c

1 file changed

Lines changed: 101 additions & 41 deletions

File tree

‎src/Imazen.WebP/AnimDecoder.cs‎

Lines changed: 101 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -54,41 +54,76 @@ public AnimDecoder(byte[] webpData, bool useThreads = false)
5454
if (webpData == null) throw new ArgumentNullException(nameof(webpData));
5555

5656
_dataHandle = GCHandle.Alloc(webpData, GCHandleType.Pinned);
57-
var data = new WebPData
57+
try
5858
{
59-
bytes = _dataHandle.AddrOfPinnedObject(),
60-
size = (UIntPtr)webpData.Length
61-
};
59+
var data = new WebPData
60+
{
61+
bytes = _dataHandle.AddrOfPinnedObject(),
62+
size = (UIntPtr)webpData.Length
63+
};
6264

63-
var options = new WebPAnimDecoderOptions();
64-
NativeLibraryLoader.FixDllNotFoundException("webpdemux", () =>
65-
{
66-
if (NativeMethods.WebPAnimDecoderOptionsInit(ref options) == 0)
67-
throw new Exception("Failed to initialize animation decoder options (version mismatch)");
68-
return 0;
69-
});
70-
options.color_mode = WEBP_CSP_MODE.MODE_BGRA;
71-
options.use_threads = useThreads ? 1 : 0;
65+
var options = new WebPAnimDecoderOptions();
66+
NativeLibraryLoader.FixDllNotFoundException("webpdemux", () =>
67+
{
68+
if (NativeMethods.WebPAnimDecoderOptionsInit(ref options) == 0)
69+
throw new Exception("Failed to initialize animation decoder options (version mismatch)");
70+
return 0;
71+
});
72+
options.color_mode = WEBP_CSP_MODE.MODE_BGRA;
73+
options.use_threads = useThreads ? 1 : 0;
7274

73-
_decoder = NativeLibraryLoader.FixDllNotFoundException("webpdemux",
74-
() => NativeMethods.WebPAnimDecoderNew(ref data, ref options));
75+
_decoder = NativeLibraryLoader.FixDllNotFoundException("webpdemux",
76+
() => NativeMethods.WebPAnimDecoderNew(ref data, ref options));
7577

76-
if (_decoder == IntPtr.Zero)
77-
throw new Exception("Failed to create animation decoder. Data may not be a valid animated WebP.");
78+
if (_decoder == IntPtr.Zero)
79+
throw new InvalidDataException("Failed to create animation decoder. Data may not be a valid animated WebP.");
7880

79-
var animInfo = new WebPAnimInfo();
80-
int infoResult = NativeLibraryLoader.FixDllNotFoundException("webpdemux",
81-
() => NativeMethods.WebPAnimDecoderGetInfo(_decoder, ref animInfo));
81+
var animInfo = new WebPAnimInfo();
82+
int infoResult = NativeLibraryLoader.FixDllNotFoundException("webpdemux",
83+
() => NativeMethods.WebPAnimDecoderGetInfo(_decoder, ref animInfo));
8284

83-
if (infoResult == 0)
84-
throw new Exception("Failed to get animation info");
85+
if (infoResult == 0)
86+
throw new InvalidDataException("Failed to get animation info");
8587

86-
_info = new AnimInfo(
87-
(int)animInfo.canvas_width,
88-
(int)animInfo.canvas_height,
89-
(int)animInfo.frame_count,
90-
(int)animInfo.loop_count,
91-
animInfo.bgcolor);
88+
// libwebp's canvas_{width,height} are uint32. Reject anything
89+
// outside the spec cap so subsequent `width * height * 4`
90+
// multiplications cannot overflow int32 silently.
91+
if (animInfo.canvas_width == 0 || animInfo.canvas_height == 0 ||
92+
animInfo.canvas_width > NativeMethods.WEBP_MAX_DIMENSION ||
93+
animInfo.canvas_height > NativeMethods.WEBP_MAX_DIMENSION)
94+
{
95+
throw new InvalidDataException(
96+
$"Animation canvas dimensions out of range: {animInfo.canvas_width}x{animInfo.canvas_height}");
97+
}
98+
99+
_info = new AnimInfo(
100+
(int)animInfo.canvas_width,
101+
(int)animInfo.canvas_height,
102+
(int)animInfo.frame_count,
103+
(int)animInfo.loop_count,
104+
animInfo.bgcolor);
105+
}
106+
catch
107+
{
108+
// Constructor exception: clean up native + pinned-handle
109+
// resources before propagating, so the GC won't pin webpData
110+
// forever and the native decoder won't leak.
111+
if (_decoder != IntPtr.Zero)
112+
{
113+
try
114+
{
115+
NativeMethods.WebPAnimDecoderDelete(_decoder);
116+
}
117+
catch
118+
{
119+
// best-effort
120+
}
121+
_decoder = IntPtr.Zero;
122+
}
123+
if (_dataHandle.IsAllocated)
124+
_dataHandle.Free();
125+
throw;
126+
}
92127
}
93128

94129
/// <summary>
@@ -140,7 +175,14 @@ public List<AnimFrame> DecodeAllFrames()
140175

141176
if (result == 0) return null;
142177

143-
int canvasSize = _info.Width * _info.Height * 4;
178+
// Width/Height are validated in the constructor to be in
179+
// (0, WEBP_MAX_DIMENSION], so canvasSize <= 16383*16383*4 fits
180+
// in int32 by an order of magnitude. The long widening here is
181+
// defense-in-depth in case the constraint is ever relaxed.
182+
long canvasSizeL = (long)_info.Width * _info.Height * 4;
183+
if (canvasSizeL <= 0 || canvasSizeL > int.MaxValue)
184+
throw new InvalidDataException($"Frame size overflow: {canvasSizeL}");
185+
int canvasSize = (int)canvasSizeL;
144186
byte[] pixels = new byte[canvasSize];
145187
Marshal.Copy(buf, pixels, 0, canvasSize);
146188

@@ -200,22 +242,40 @@ private static byte[] ReadStreamFully(Stream stream)
200242

201243
public void Dispose()
202244
{
203-
if (!_disposed)
245+
Dispose(true);
246+
GC.SuppressFinalize(this);
247+
}
248+
249+
~AnimDecoder()
250+
{
251+
Dispose(false);
252+
}
253+
254+
private void Dispose(bool disposing)
255+
{
256+
if (_disposed) return;
257+
258+
if (_decoder != IntPtr.Zero)
204259
{
205-
if (_decoder != IntPtr.Zero)
260+
// Finalizer path: don't go through FixDllNotFoundException —
261+
// it can take locks and call back into managed code, which
262+
// is unsafe from a finalizer thread. The native library is
263+
// already loaded by this point (the decoder handle came
264+
// from it), so a direct call is fine.
265+
try
206266
{
207-
NativeLibraryLoader.FixDllNotFoundException("webpdemux", () =>
208-
{
209-
NativeMethods.WebPAnimDecoderDelete(_decoder);
210-
return 0;
211-
});
212-
_decoder = IntPtr.Zero;
267+
NativeMethods.WebPAnimDecoderDelete(_decoder);
213268
}
214-
if (_dataHandle.IsAllocated)
215-
_dataHandle.Free();
216-
217-
_disposed = true;
269+
catch
270+
{
271+
// best-effort
272+
}
273+
_decoder = IntPtr.Zero;
218274
}
275+
if (_dataHandle.IsAllocated)
276+
_dataHandle.Free();
277+
278+
_disposed = true;
219279
}
220280
}
221281
}

0 commit comments

Comments
 (0)