Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix suspicious code fragments #112384

Merged
merged 2 commits into from
Feb 12, 2025
+13 −22
Merged

Fix suspicious code fragments #112384

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99fd329
Fix suspicious code fragments
jkotas Feb 11, 2025
7f137e2
Update src/libraries/System.Reflection.Emit/src/System/Reflection/Emi…
jkotas Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/coreclr/tools/ILVerification/ILImporter.StackValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,6 @@ static public StackValue CreateFromType(TypeDesc type)

public override bool Equals(object obj)
{
if (Object.ReferenceEquals(this, obj))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always false

return true;

if (!(obj is StackValue))
return false;

Expand Down
2 changes: 1 addition & 1 deletion ...icrosoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/DynamicVariantExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public static void SetAsIConvertible(this ref ComVariant variant, IConvertible v
case TypeCode.Int32: variant = ComVariant.Create(value.ToInt32(ci)); break;
case TypeCode.UInt32: variant = ComVariant.Create(value.ToUInt32(ci)); break;
case TypeCode.Int64: variant = ComVariant.Create(value.ToInt64(ci)); break;
case TypeCode.UInt64: variant = ComVariant.Create(value.ToInt64(ci)); break;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a real bug

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a long-standing bug from at least .NET 6, likely .NET Framework (as it's in the DLR version this code was initially ported from as well):

runtime/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/Variant.Extended.cs

Line 307 in f1dd571

case TypeCode.UInt64: AsI8 = value.ToInt64(ci); break;

I think we can fix this going forward, but as this does change the variant type I'd be concerned of breaking consumers if we were to backport this fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I do not think this is something that we want to backport.

case TypeCode.UInt64: variant = ComVariant.Create(value.ToUInt64(ci)); break;
case TypeCode.Single: variant = ComVariant.Create(value.ToSingle(ci)); break;
case TypeCode.Double: variant = ComVariant.Create(value.ToDouble(ci)); break;
case TypeCode.Decimal: variant = ComVariant.Create(value.ToDecimal(ci)); break;
Expand Down
3 changes: 0 additions & 3 deletions src/libraries/System.CodeDom/src/Microsoft/VisualBasic/VBCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,9 +577,6 @@ protected override void OutputMemberScopeModifier(MemberAttributes attributes)
case MemberAttributes.Override:
Output.Write("Overrides ");
break;
case MemberAttributes.Private:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreachable code. Legacy components - keeping the existing behavior.

Output.Write("Private ");
break;
default:
switch (attributes & MemberAttributes.AccessMask)
{
Expand Down
27 changes: 12 additions & 15 deletions src/libraries/System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3023,22 +3023,19 @@ private void ImplReadData(BinXmlToken tokenType)

private void ImplReadElement()
{
if (3 != _docState || 9 != _docState)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always true.

switch (_docState)
{
switch (_docState)
{
case 0:
_docState = 9;
break;
case 1:
case 2:
_docState = 3;
break;
case -1:
throw CreateUnexpectedTokenException(_token);
default:
break;
}
case 0:
_docState = 9;
break;
case 1:
case 2:
_docState = 3;
break;
case -1:
throw CreateUnexpectedTokenException(_token);
default:
break;
}
_elemDepth++;
if (_elemDepth == _elementStack.Length)
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ private static bool MatchesTheFilter(MethodBuilderImpl method, BindingFlags meth

if (argType.IsArray != paramType.IsArray ||
argType.IsByRef != paramType.IsByRef ||
argType.IsPointer != argType.IsPointer)
Copy link
Member Author

@jkotas jkotas Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a real corner case bug, but I do not think it is worth it to add a test case that hits it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#112048

argType.IsPointer != paramType.IsPointer)
{
return false;
}
Expand Down
Loading