runtime: minor bugs fixed in coreclr and libraries #97155

kaybelev · 2024-01-18T14:42:57Z

The bugs fixed are related to:
-situations where a pointer is dereferenced, but can only have NULL value, and so the operation of dereference can never be run without causing a runtime error;
-after some value is retrieved by as operator, the original value is checked for null instead of the new one.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

…cpp:262, may be NULL and is dereferenced at filetime.cpp:268>. The error was detected using the Svace analyzer maded by ISP RAS.

…d at process.cpp:2824 without checking for NULL, but it is usually checked for this function (11/12)>. The error was detected using the Svace analyzer maded by ISP RAS.

…on with possible null return value, is dereferenced in member access expression sectionRecord.HasLocationInput>. The error was detected using the Svace analyzer maded by ISP RAS.

…h AS-cast should be compared with null instead of method>. The error was detected using the Svace analyzer maded by ISP RAS.

kaybelev · 2024-01-18T14:45:42Z

@dotnet-policy-service agree

stephentoub · 2024-01-18T14:52:39Z

...ystem.Configuration.ConfigurationManager/src/System/Configuration/MgmtConfigurationRecord.cs

@@ -335,7 +335,7 @@ internal ConfigurationSection FindImmediateParentSection(ConfigurationSection se

            string configKey = section.SectionInformation.SectionName;
            SectionRecord sectionRecord = GetSectionRecord(configKey, false);
-            if (sectionRecord.HasLocationInputs)
+            if ((sectionRecord != null) && sectionRecord.HasLocationInputs)


This doesn't actually fix a null reference exception. If sectionRecord could actually be null, this will cause the else block to be executed, which also immediately dereferences sectionRecord.

Unless there's a known repro for this causing a problem, let's revert this.

Stefan, to correct the error, I propose below in line 346 to change the condition to <if ((sectionRecord != null) && sectionRecord.HasIndirectLocationInputs)>. Stefan, judging by the results of the static analysis, the value returned by the function GetSectionRecord(...) in the library System.Configuration.ConfigurationManager code base should be checked for NULL (you can search (sectionRecord==null or sectionRecord!=null) by searching in the files MgmtConfigurationRecord.cs and BaseConfigurationRecord.cs). I suggest you fix it. It is not possible to check for an error in this case using a test, because it was detected using a static analysis tool.

Commit updated.

This method is searching for immediate parent section. I assume that the immediate parent section is expected to always exist and that's why there is no null check. Maybe we can add Debug.Assert instead?

Yes, let's try, just explain what needs to be done for this.

src/coreclr/pal/src/thread/process.cpp

jkotas · 2024-01-18T15:15:06Z

src/coreclr/pal/src/file/filetime.cpp

@@ -263,6 +263,7 @@ BOOL PALAPI FileTimeToSystemTime( CONST FILETIME * lpFileTime,
 #else   /* HAVE_GMTIME_R */
        UnixSystemTime = gmtime( &UnixFileTime );
 #endif  /* HAVE_GMTIME_R */
+        if (!UnixSystemTime) return FALSE;


Suggested change

if (!UnixSystemTime) return FALSE;

if (!UnixSystemTime)

{

ERROR( "gmtime failed.\n" );

SetLastError(ERROR_INVALID_PARAMETER);

return FALSE;

}

Jan, thanks for the correction, we'll take it into account for the future.

Commit updated.

...ystem.Configuration.ConfigurationManager/src/System/Configuration/MgmtConfigurationRecord.cs

…stem/Configuration/MgmtConfigurationRecord.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

jkotas

Thanks

kaybelev · 2024-01-22T12:08:54Z

Thanks for the work you've done.

* Fixed error: <Pointer, returned from function 'gmtime_r' at filetime.cpp:262, may be NULL and is dereferenced at filetime.cpp:268>. The error was detected using the Svace analyzer maded by ISP RAS. * Fixed error: <Return value of a function 'PAL_wcsrchr' is dereferenced at process.cpp:2824 without checking for NULL, but it is usually checked for this function (11/12)>. The error was detected using the Svace analyzer maded by ISP RAS. * Fixed error: <Value sectionRecord, which is result of method invocation with possible null return value, is dereferenced in member access expression sectionRecord.HasLocationInput>. The error was detected using the Svace analyzer maded by ISP RAS. * Fixed error: <Maybe ecmaCandidateMethod, that created from method with AS-cast should be compared with null instead of method>. The error was detected using the Svace analyzer maded by ISP RAS. * Update filetime.cpp * Update process.cpp * Update MgmtConfigurationRecord.cs * Update src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/MgmtConfigurationRecord.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com> * Update src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/MgmtConfigurationRecord.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com> --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>

kaybelev added 4 commits January 18, 2024 11:38

Fixed error: <Pointer, returned from function 'gmtime_r' at filetime.…

5fcb82d

…cpp:262, may be NULL and is dereferenced at filetime.cpp:268>. The error was detected using the Svace analyzer maded by ISP RAS.

Fixed error: <Return value of a function 'PAL_wcsrchr' is dereference…

09fd2f5

…d at process.cpp:2824 without checking for NULL, but it is usually checked for this function (11/12)>. The error was detected using the Svace analyzer maded by ISP RAS.

Fixed error: <Value sectionRecord, which is result of method invocati…

3369cbe

…on with possible null return value, is dereferenced in member access expression sectionRecord.HasLocationInput>. The error was detected using the Svace analyzer maded by ISP RAS.

Fixed error: <Maybe ecmaCandidateMethod, that created from method wit…

d32b505

…h AS-cast should be compared with null instead of method>. The error was detected using the Svace analyzer maded by ISP RAS.

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 18, 2024

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 Jan 18, 2024

stephentoub reviewed Jan 18, 2024

View reviewed changes

jkotas reviewed Jan 18, 2024

View reviewed changes

src/coreclr/pal/src/thread/process.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jan 18, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jan 18, 2024

BasicEventSourceTests.TestsManifestGeneration test failing in CI #97103

Closed

kaybelev requested a review from stephentoub January 19, 2024 10:24

kaybelev added 3 commits January 19, 2024 13:47

Update filetime.cpp

82d4ff2

Update process.cpp

202117d

Update MgmtConfigurationRecord.cs

41ed108

kaybelev requested a review from jkotas January 19, 2024 11:04

jkotas reviewed Jan 19, 2024

View reviewed changes

...ystem.Configuration.ConfigurationManager/src/System/Configuration/MgmtConfigurationRecord.cs Outdated Show resolved Hide resolved

jkotas reviewed Jan 19, 2024

View reviewed changes

...ystem.Configuration.ConfigurationManager/src/System/Configuration/MgmtConfigurationRecord.cs Outdated Show resolved Hide resolved

kaybelev and others added 2 commits January 19, 2024 14:48

Update src/libraries/System.Configuration.ConfigurationManager/src/Sy…

11e19ea

…stem/Configuration/MgmtConfigurationRecord.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Update src/libraries/System.Configuration.ConfigurationManager/src/Sy…

a7282df

…stem/Configuration/MgmtConfigurationRecord.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

jkotas approved these changes Jan 19, 2024

View reviewed changes

jkotas merged commit 43f2d94 into dotnet:main Jan 19, 2024
146 of 151 checks passed

github-actions bot locked and limited conversation to collaborators Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime: minor bugs fixed in coreclr and libraries #97155

runtime: minor bugs fixed in coreclr and libraries #97155

kaybelev commented Jan 18, 2024 •

edited

Loading

kaybelev commented Jan 18, 2024

stephentoub Jan 18, 2024

kaybelev Jan 19, 2024

kaybelev Jan 19, 2024 •

edited

Loading

jkotas Jan 19, 2024

kaybelev Jan 19, 2024

jkotas Jan 18, 2024

kaybelev Jan 19, 2024

kaybelev Jan 19, 2024

jkotas left a comment

kaybelev commented Jan 22, 2024

runtime: minor bugs fixed in coreclr and libraries #97155

runtime: minor bugs fixed in coreclr and libraries #97155

Conversation

kaybelev commented Jan 18, 2024 • edited Loading

kaybelev commented Jan 18, 2024

stephentoub Jan 18, 2024

Choose a reason for hiding this comment

kaybelev Jan 19, 2024

Choose a reason for hiding this comment

kaybelev Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

jkotas Jan 19, 2024

Choose a reason for hiding this comment

kaybelev Jan 19, 2024

Choose a reason for hiding this comment

jkotas Jan 18, 2024

Choose a reason for hiding this comment

kaybelev Jan 19, 2024

Choose a reason for hiding this comment

kaybelev Jan 19, 2024

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

kaybelev commented Jan 22, 2024

kaybelev commented Jan 18, 2024 •

edited

Loading

kaybelev Jan 19, 2024 •

edited

Loading