From 06fa9410a86ebed2ccc789f1859489af394d0d8c Mon Sep 17 00:00:00 2001 From: buyaa-n Date: Wed, 1 Jul 2020 18:36:56 -0700 Subject: [PATCH] Turn on argument exception analyzer on runtime, fix warnings found (#38578) * Turn on argument exception analyzer on runtime, fix failures found --- .editorconfig | 2 +- eng/CodeAnalysis.ruleset | 2 +- .../Reflection/Emit/CustomAttributeBuilder.cs | 2 ++ .../src/System/Drawing/Bitmap.Unix.cs | 2 +- .../src/System/Drawing/Bitmap.Windows.cs | 3 +++ .../System/Drawing/Drawing2D/GraphicsPath.Unix.cs | 12 ++++++------ .../Drawing/Drawing2D/GraphicsPath.Windows.cs | 14 ++++++++++++++ .../src/System/Drawing/Icon.Unix.cs | 8 ++++---- .../src/System/Drawing/Icon.Windows.cs | 14 +++++++++++++- .../System.Drawing.Common/tests/BitmapTests.cs | 7 ++++++- .../tests/Drawing2D/GraphicsPathTests.cs | 8 ++++---- .../System.Drawing.Common/tests/IconTests.cs | 11 ++++++++--- .../tests/mono/System.Drawing/BitmapTests.cs | 2 +- 13 files changed, 64 insertions(+), 23 deletions(-) diff --git a/.editorconfig b/.editorconfig index 4b75cbf9ed140..f686aa1783f78 100644 --- a/.editorconfig +++ b/.editorconfig @@ -155,7 +155,7 @@ csharp_space_between_square_brackets = false # Analyzers dotnet_code_quality.ca1802.api_surface = private, internal -dotnet_code_quality.CA2208.api_surface = public +dotnet_code_quality.ca2208.api_surface = public # C++ Files [*.{cpp,h,in}] diff --git a/eng/CodeAnalysis.ruleset b/eng/CodeAnalysis.ruleset index 76dafbee5b816..e75b9b90d350d 100644 --- a/eng/CodeAnalysis.ruleset +++ b/eng/CodeAnalysis.ruleset @@ -131,7 +131,7 @@ - + diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.cs index d1e89aaf0c3bf..7b078fe7671df 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.cs @@ -63,10 +63,12 @@ public CustomAttributeBuilder(ConstructorInfo con, object?[] constructorArgs, Pr throw new ArgumentNullException(nameof(namedFields)); if (fieldValues == null) throw new ArgumentNullException(nameof(fieldValues)); +#pragma warning disable CA2208 // Instantiate argument exceptions correctly, combination of arguments used if (namedProperties.Length != propertyValues.Length) throw new ArgumentException(SR.Arg_ArrayLengthsDiffer, "namedProperties, propertyValues"); if (namedFields.Length != fieldValues.Length) throw new ArgumentException(SR.Arg_ArrayLengthsDiffer, "namedFields, fieldValues"); +#pragma warning restore CA2208 if ((con.Attributes & MethodAttributes.Static) == MethodAttributes.Static || (con.Attributes & MethodAttributes.MemberAccessMask) == MethodAttributes.Private) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Unix.cs index 195e01ac5f956..a04ddfd50ce55 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Unix.cs @@ -67,7 +67,7 @@ public Bitmap(Stream stream, bool useIcm) public Bitmap(Type type, string resource) { if (resource == null) - throw new ArgumentException(nameof(resource)); + throw new ArgumentNullException(nameof(resource)); // For compatibility with the .NET Framework if (type == null) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Windows.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Windows.cs index c879bf78d0d2f..10120614c6a6b 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Windows.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Windows.cs @@ -12,6 +12,9 @@ public sealed partial class Bitmap { public Bitmap(Type type, string resource) { + if (resource == null) + throw new ArgumentNullException(nameof(resource)); + Stream? stream = type.Module.Assembly.GetManifestResourceStream(type, resource); if (stream == null) { diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Unix.cs index 5234e8d5712a7..9a125a76d8e6c 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Unix.cs @@ -381,7 +381,7 @@ public void AddLines(Point[] points) if (points == null) throw new ArgumentNullException(nameof(points)); if (points.Length == 0) - throw new ArgumentException(nameof(points)); + throw new ArgumentException(null, nameof(points)); int status = Gdip.GdipAddPathLine2I(_nativePath, points, points.Length); Gdip.CheckStatus(status); @@ -392,7 +392,7 @@ public void AddLines(PointF[] points) if (points == null) throw new ArgumentNullException(nameof(points)); if (points.Length == 0) - throw new ArgumentException(nameof(points)); + throw new ArgumentException(null, nameof(points)); int status = Gdip.GdipAddPathLine2(_nativePath, points, points.Length); Gdip.CheckStatus(status); @@ -464,7 +464,7 @@ public void AddRectangles(Rectangle[] rects) if (rects == null) throw new ArgumentNullException(nameof(rects)); if (rects.Length == 0) - throw new ArgumentException(nameof(rects)); + throw new ArgumentException(null, nameof(rects)); int status = Gdip.GdipAddPathRectanglesI(_nativePath, rects, rects.Length); Gdip.CheckStatus(status); @@ -475,7 +475,7 @@ public void AddRectangles(RectangleF[] rects) if (rects == null) throw new ArgumentNullException(nameof(rects)); if (rects.Length == 0) - throw new ArgumentException(nameof(rects)); + throw new ArgumentException(null, nameof(rects)); int status = Gdip.GdipAddPathRectangles(_nativePath, rects, rects.Length); Gdip.CheckStatus(status); @@ -642,7 +642,7 @@ public void AddString(string s, FontFamily family, int style, float emSize, Poin public void AddString(string s, FontFamily family, int style, float emSize, Rectangle layoutRect, StringFormat? format) { if (family == null) - throw new ArgumentException(nameof(family)); + throw new ArgumentNullException(nameof(family)); IntPtr sformat = (format == null) ? IntPtr.Zero : format.nativeFormat; // note: the NullReferenceException on s.Length is the expected (MS) exception @@ -653,7 +653,7 @@ public void AddString(string s, FontFamily family, int style, float emSize, Rect public void AddString(string s, FontFamily family, int style, float emSize, RectangleF layoutRect, StringFormat? format) { if (family == null) - throw new ArgumentException(nameof(family)); + throw new ArgumentNullException(nameof(family)); IntPtr sformat = (format == null) ? IntPtr.Zero : format.nativeFormat; // note: the NullReferenceException on s.Length is the expected (MS) exception diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs index 249b3970adb79..93017f1a199b4 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs @@ -294,6 +294,8 @@ public unsafe void AddLines(PointF[] points) { if (points == null) throw new ArgumentNullException(nameof(points)); + if (points.Length == 0) + throw new ArgumentException(null, nameof(points)); fixed (PointF* p = points) { @@ -312,6 +314,8 @@ public unsafe void AddLines(Point[] points) { if (points == null) throw new ArgumentNullException(nameof(points)); + if (points.Length == 0) + throw new ArgumentException(null, nameof(points)); fixed (Point* p = points) { @@ -526,6 +530,8 @@ public unsafe void AddRectangles(RectangleF[] rects) { if (rects == null) throw new ArgumentNullException(nameof(rects)); + if (rects.Length == 0) + throw new ArgumentException(null, nameof(rects)); fixed (RectangleF* r = rects) { @@ -545,6 +551,8 @@ public unsafe void AddRectangles(Rectangle[] rects) { if (rects == null) throw new ArgumentNullException(nameof(rects)); + if (rects.Length == 0) + throw new ArgumentException(null, nameof(rects)); fixed (Rectangle* r = rects) { @@ -639,6 +647,9 @@ public void AddString(string s, FontFamily family, int style, float emSize, Poin public void AddString(string s, FontFamily family, int style, float emSize, RectangleF layoutRect, StringFormat? format) { + if (family == null) + throw new ArgumentNullException(nameof(family)); + Gdip.CheckStatus(Gdip.GdipAddPathString( new HandleRef(this, _nativePath), s, @@ -652,6 +663,9 @@ public void AddString(string s, FontFamily family, int style, float emSize, Rect public void AddString(string s, FontFamily family, int style, float emSize, Rectangle layoutRect, StringFormat? format) { + if (family == null) + throw new ArgumentNullException(nameof(family)); + Gdip.CheckStatus(Gdip.GdipAddPathStringI( new HandleRef(this, _nativePath), s, diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Unix.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Unix.cs index 96c81dab0eded..3553957cb5faa 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Unix.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Unix.cs @@ -205,7 +205,7 @@ public Icon(Icon original, Size size) } if (id == ushort.MaxValue) - throw new ArgumentException(SR.NoValidIconImageFound, "Icon"); + throw new ArgumentException(SR.NoValidIconImageFound, nameof(original)); iconSize.Height = iconDir.idEntries[id].height; iconSize.Width = iconDir.idEntries[id].width; @@ -240,7 +240,7 @@ public Icon(string fileName) public Icon(Type type, string resource) { if (resource == null) - throw new ArgumentException(nameof(resource)); + throw new ArgumentNullException(nameof(resource)); // For compatibility with the .NET Framework if (type == null) @@ -313,7 +313,7 @@ public static Icon ExtractAssociatedIcon(string filePath) if (filePath == null) throw new ArgumentNullException(nameof(filePath)); if (string.IsNullOrEmpty(filePath)) - throw new ArgumentException(SR.NullOrEmptyPath, "path"); + throw new ArgumentException(SR.NullOrEmptyPath, nameof(filePath)); if (!File.Exists(filePath)) throw new FileNotFoundException(SR.CouldntFindSpecifiedFile, filePath); @@ -346,7 +346,7 @@ public object Clone() public static Icon FromHandle(IntPtr handle) { if (handle == IntPtr.Zero) - throw new ArgumentException(nameof(handle)); + throw new ArgumentException(null, nameof(handle)); return new Icon(handle); } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs index 4b983764be28a..9d583846c47d9 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs @@ -104,6 +104,9 @@ public Icon(Icon original, int width, int height) : this() public Icon(Type type, string resource) : this() { + if (resource == null) + throw new ArgumentNullException(nameof(resource)); + Stream? stream = type.Module.Assembly.GetManifestResourceStream(type, resource); if (stream == null) { @@ -167,6 +170,9 @@ void ISerializable.GetObjectData(SerializationInfo si, StreamingContext context) throw new ArgumentNullException(nameof(filePath)); } + if (string.IsNullOrEmpty(filePath)) + throw new ArgumentException(SR.NullOrEmptyPath, nameof(filePath)); + filePath = Path.GetFullPath(filePath); if (!File.Exists(filePath)) { @@ -440,7 +446,13 @@ internal void DrawUnstretched(Graphics graphics, Rectangle targetRect) ~Icon() => Dispose(false); - public static Icon FromHandle(IntPtr handle) => new Icon(handle); + public static Icon FromHandle(IntPtr handle) + { + if (handle == IntPtr.Zero) + throw new ArgumentException(null, nameof(handle)); + + return new Icon(handle); + } // Initializes this Image object. This is identical to calling the image's // constructor with picture, but this allows non-constructor initialization, diff --git a/src/libraries/System.Drawing.Common/tests/BitmapTests.cs b/src/libraries/System.Drawing.Common/tests/BitmapTests.cs index 9c5ca385b9f99..351db2275fa54 100644 --- a/src/libraries/System.Drawing.Common/tests/BitmapTests.cs +++ b/src/libraries/System.Drawing.Common/tests/BitmapTests.cs @@ -111,7 +111,6 @@ public void Ctor_NullType_ThrowsNullReferenceException() [ActiveIssue("https://github.com/dotnet/runtime/issues/22221", TestPlatforms.AnyUnix)] [ConditionalTheory(Helpers.IsDrawingSupported)] - [InlineData(typeof(Bitmap), null)] [InlineData(typeof(Bitmap), "")] [InlineData(typeof(Bitmap), "bitmap_173x183_indexed_8bit.bmp")] [InlineData(typeof(BitmapTests), "bitmap_173x183_INDEXED_8bit.bmp")] @@ -121,6 +120,12 @@ public void Ctor_InvalidResource_ThrowsArgumentException(Type type, string resou AssertExtensions.Throws(null, () => new Bitmap(type, resource)); } + [ConditionalFact(Helpers.IsDrawingSupported)] + public void Ctor_InvalidResource_ThrowsArgumentNullException() + { + AssertExtensions.Throws("resource", null, () => new Bitmap(typeof(Bitmap), null)); + } + [ConditionalTheory(Helpers.IsDrawingSupported)] [MemberData(nameof(Ctor_FilePath_TestData))] public void Ctor_Stream(string filename, int width, int height, PixelFormat pixelFormat, ImageFormat rawFormat) diff --git a/src/libraries/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs b/src/libraries/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs index 50f0e071da5b1..1eef306d6b13c 100644 --- a/src/libraries/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs +++ b/src/libraries/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs @@ -370,8 +370,8 @@ public void AddLines_PointsNull_ThrowsArgumentNullException() [ConditionalFact(Helpers.IsDrawingSupported)] public void AddLines_ZeroPoints_ThrowsArgumentException() { - AssertExtensions.Throws(null, () => new GraphicsPath().AddLines(new Point[0])); - AssertExtensions.Throws(null, () => new GraphicsPath().AddLines(new PointF[0])); + AssertExtensions.Throws("points", null, () => new GraphicsPath().AddLines(new Point[0])); + AssertExtensions.Throws("points", null, () => new GraphicsPath().AddLines(new PointF[0])); } [ActiveIssue("https://github.com/dotnet/runtime/issues/22221", TestPlatforms.AnyUnix)] @@ -1262,11 +1262,11 @@ public void AddString_StringNull_ThrowsNullReferenceException() } [ConditionalFact(Helpers.IsDrawingSupported)] - public void AddString_FontFamilyNull_ThrowsArgumentException() + public void AddString_FontFamilyNull_ThrowsArgumentNullException() { using (GraphicsPath gp = new GraphicsPath()) { - AssertExtensions.Throws(null, () => + AssertExtensions.Throws("family", null, () => new GraphicsPath().AddString("mono", null, 0, 10, new Point(10, 10), StringFormat.GenericDefault)); } } diff --git a/src/libraries/System.Drawing.Common/tests/IconTests.cs b/src/libraries/System.Drawing.Common/tests/IconTests.cs index 8389741be2916..22a01301b65e9 100644 --- a/src/libraries/System.Drawing.Common/tests/IconTests.cs +++ b/src/libraries/System.Drawing.Common/tests/IconTests.cs @@ -283,7 +283,6 @@ public void Ctor_NullType_ThrowsNullReferenceException() } [ConditionalTheory(Helpers.IsDrawingSupported)] - [InlineData(typeof(Icon), null)] [InlineData(typeof(Icon), "")] [InlineData(typeof(Icon), "48x48_multiple_entries_4bit.ico")] [InlineData(typeof(IconTests), "48x48_MULTIPLE_entries_4bit.ico")] @@ -292,6 +291,12 @@ public void Ctor_InvalidResource_ThrowsArgumentException(Type type, string resou AssertExtensions.Throws(null, () => new Icon(type, resource)); } + [ConditionalFact(Helpers.IsDrawingSupported)] + public void Ctor_InvalidResource_ThrowsArgumentNullException() + { + AssertExtensions.Throws("resource", null, () => new Icon(typeof(Icon), null)); + } + [ConditionalFact(Helpers.IsDrawingSupported)] public void Clone_ConstructedIcon_Success() { @@ -437,7 +442,7 @@ public void ExtractAssociatedIcon_NullFilePath_ThrowsArgumentNullException() [ConditionalFact(Helpers.IsDrawingSupported)] public void ExtractAssociatedIcon_InvalidFilePath_ThrowsArgumentException() { - AssertExtensions.Throws("path", null, () => Icon.ExtractAssociatedIcon("")); + AssertExtensions.Throws("filePath", null, () => Icon.ExtractAssociatedIcon("")); } @@ -759,7 +764,7 @@ public void FromHandle_BitmapHandleMultipleTime_Success() [ConditionalFact(Helpers.IsDrawingSupported)] public void FromHandle_Zero_ThrowsArgumentException() { - AssertExtensions.Throws(null, () => Icon.FromHandle(IntPtr.Zero)); + AssertExtensions.Throws("handle", null, () => Icon.FromHandle(IntPtr.Zero)); } [ConditionalFact(Helpers.IsDrawingSupported)] diff --git a/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/BitmapTests.cs b/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/BitmapTests.cs index a2f495e0cbe56..689bb3f9e4ee3 100644 --- a/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/BitmapTests.cs +++ b/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/BitmapTests.cs @@ -1304,7 +1304,7 @@ public void BitmapTypeStringCtor1() [ConditionalFact(Helpers.IsDrawingSupported)] public void BitmapTypeStringCtor2() { - Assert.Throws(() => new Bitmap(typeof(Bitmap), null)); + AssertExtensions.Throws("resource", null, () => new Bitmap(typeof(Bitmap), null)); } private void SetResolution(float x, float y)