Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Fix suspicious code fragments #112384

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Fix suspicious code fragments #112384

merged 2 commits into from
Feb 12, 2025

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Feb 11, 2025

Contributes to #112344

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 11, 2025
jkotas
jkotas commented Feb 11, 2025
View reviewed changes
...s/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/DynamicVariantExtensions.cs
@@ -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.

jkotas
jkotas commented Feb 11, 2025
View reviewed changes
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs Outdated
@@ -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

jkotas
jkotas commented Feb 11, 2025
View reviewed changes
src/coreclr/tools/ILVerification/ILImporter.StackValue.cs
@@ -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

jkotas
jkotas commented Feb 11, 2025
View reviewed changes
src/libraries/System.CodeDom/src/Microsoft/VisualBasic/VBCodeGenerator.cs
@@ -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.

jkotas
jkotas commented Feb 11, 2025
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs
@@ -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.

@jkotas jkotas added area-System.Runtime.InteropServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 11, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@jkotas jkotas mentioned this pull request Feb 11, 2025
Open
This was referenced Feb 11, 2025
Open
Open
Open
Open
Open
Open
Open
This was referenced Feb 12, 2025
Open
Open
Open
Open
@jkotas
Copy link
Member Author

jkotas commented Feb 12, 2025

/ba-g unrelated timeouts due to overloaded queues

@jkotas jkotas merged commit 4008de1 into dotnet:main Feb 12, 2025
126 of 137 checks passed
@jkotas jkotas deleted the issue-112344 branch February 12, 2025 06:39
grendello added a commit to grendello/runtime that referenced this pull request Feb 12, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
21f8a18 
* main:
  [Android] Run CoreCLR functional tests on Android (dotnet#112283)
  [LoongArch64] Fix some assertion failures for Debug ILC building Debug NativeAOT testcases. (dotnet#112229)
  Fix suspicious code fragments (dotnet#112384)
  `__ComObject` doesn't support dynamic interface map (dotnet#112375)
  Native DLLs: only load imported DLLs from System32 (dotnet#112359)
  [main] Update dependencies from dotnet/roslyn (dotnet#112314)
  Update SVE instructions that writes to GC regs (dotnet#112389)
  Bring up android+coreclr windows build.  (dotnet#112256)
  Never use heap for return buffers (dotnet#112060)
  Wait to complete the test before releasing the agile reference. (dotnet#112387)
  Prevent returning disposed HTTP/1.1 connections to the pool (dotnet#112383)
  Fingerprint dotnet.js if writing import map to html is enabled (dotnet#112407)
  Remove duplicate definition of CORECLR_HOSTING_API_LINKAGE (dotnet#112096)
  Update the exception message to reflect current behavior. (dotnet#112355)
  Use enum for frametype not v table (dotnet#112166)
  Enable AltJits build for LoongArch64 and RiscV64 (dotnet#110282)
  Guard members of MonoType union & fix related bugs (dotnet#111645)
  Add optional hooks for debugging OpenSSL memory allocations (dotnet#111539)
  JIT: Optimize struct parameter register accesses in the backend (dotnet#110819)
  NativeAOT: Cover more opcodes in type preinitializer (dotnet#112073)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lewing lewing lewing left review comments

@filipnavara filipnavara filipnavara approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@jkoritzinsky jkoritzinsky Awaiting requested review from jkoritzinsky

Assignees

@jkotas jkotas

Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jkotas @lewing @jkoritzinsky @filipnavara @AaronRobinsonMSFT