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.

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

Fix condition in Debug.Assert in TarEntry.cs #100860

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Conversation

usr-sse2
Copy link
Contributor

@usr-sse2 usr-sse2 commented Apr 10, 2024

The precedence of not is higher than of or, so without parentheses the actual condition was ((not RegularFile) or V7RegularFile or ContigousFile), which is not what this function actually expects (see switch cases and the condition in caller function).

The issue was found by Svace static analyzer: https://www.ispras.ru/en/technologies/svace/

The precedence of `not` is higher than of `or`, so without parentheses the actual condition was `((not RegularFile) or V7RegularFile or ContigousFile)`, which is not what this function actually expects (see switch cases and the condition in caller function).
Copy link
Contributor

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

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 10, 2024
@usr-sse2 usr-sse2 requested a review from marek-safar as a code owner April 10, 2024 11:18
@@ -424,7 +424,7 @@ private Task ExtractToFileInternalAsync(string filePath, string? linkTargetPath,

private void CreateNonRegularFile(string filePath, string? linkTargetPath)
{
Debug.Assert(EntryType is not TarEntryType.RegularFile or TarEntryType.V7RegularFile or TarEntryType.ContiguousFile);
Debug.Assert(EntryType is not (TarEntryType.RegularFile or TarEntryType.V7RegularFile or TarEntryType.ContiguousFile));
Copy link
Member

Choose a reason for hiding this comment

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

nit: alternative fix is what we are using in number of places (including this file):

Suggested change
Debug.Assert(EntryType is not (TarEntryType.RegularFile or TarEntryType.V7RegularFile or TarEntryType.ContiguousFile));
Debug.Assert(EntryType is not TarEntryType.RegularFile and not TarEntryType.V7RegularFile or TarEntryType.ContiguousFile);

@am11
Copy link
Member

am11 commented Apr 10, 2024

Good catch! C# doesn't have nor in its vocabulary (yet). 😅

There is another instance in the repo:

Debug.Assert (provider is not ModuleDefinition or AssemblyDefinition);

could you fix it as well while at it?

@usr-sse2
Copy link
Contributor Author

Good catch! C# doesn't have nor in its vocabulary (yet). 😅

There is another instance in the repo:

Debug.Assert (provider is not ModuleDefinition or AssemblyDefinition);

could you fix it as well while at it?

I fixed it before you asked (by adding second commit into this pull request).

Regarding nor, I've thought of another way to express it:

Debug.Assert (provider isn't ModuleDefinition or AssemblyDefinition);

where isn't is a single operator, which is applied to the whole expression, unlike is not.

@stephentoub stephentoub merged commit f3ee529 into dotnet:main Jul 22, 2024
104 of 108 checks passed
@stephentoub
Copy link
Member

Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Tar community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants