From c1f3d38055983e23765ace8ab122a84b548220db Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Tue, 6 Aug 2024 16:55:23 +0200 Subject: [PATCH 1/2] Modifiy S2674: Promote it to SonarWay --- rules/S2674/csharp/metadata.json | 5 +-- rules/S2674/csharp/rule.adoc | 62 ++++++++++++++++++++------------ 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/rules/S2674/csharp/metadata.json b/rules/S2674/csharp/metadata.json index 822f09bb73f..f29277906a6 100644 --- a/rules/S2674/csharp/metadata.json +++ b/rules/S2674/csharp/metadata.json @@ -1,6 +1,3 @@ { - "title": "The length returned from a stream read should be checked", - "defaultQualityProfiles": [ - - ] + "title": "The length returned from a stream read should be checked" } diff --git a/rules/S2674/csharp/rule.adoc b/rules/S2674/csharp/rule.adoc index 46d95a39d28..d9b9d428372 100644 --- a/rules/S2674/csharp/rule.adoc +++ b/rules/S2674/csharp/rule.adoc @@ -1,47 +1,63 @@ == Why is this an issue? -You cannot assume that any given stream reading call will fill the ``++byte[]++`` passed in to the method with the number of bytes requested. Instead, you must check the value returned by the read method to see how many bytes were read. Fail to do so, and you introduce a bug that is both harmful and difficult to reproduce. +Invoking a stream reading method without verifying the number of bytes read can lead to erroneous assumptions. A Stream can represent any I/O operation, reading a file, network communication or inter-process communication, as such it is not guaranteed that the `byte[]` passed into the method will be filled with the requested number of bytes. Therefore, it is crucial to inspect the value returned by the read method to ascertain the actual number of bytes read. +Neglecting the returned length read can result in a bug that is difficult to reproduce. -This rule raises an issue when a ``++Stream.Read++`` or a ``++Stream.ReadAsync++`` method is called, but the return value is not checked. +This rule raises an issue when the returned value is ignored for the following methods: +* https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.read[Stream.Read] +* https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync[Stream.ReadAsync] +* https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readatleast[Stream.ReadAtLeast] +* https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readatleastasync[Stream.ReadAtLeastAsync] -=== Noncompliant code example +== How to fix it -[source,csharp] +Ensure to check the return value of `Stream.Read` methods to verify the actual number of bytes read, and use this value when processing the data to avoid potential bugs. + +=== Code examples + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] ---- -public void DoSomething(string fileName) +public byte[] DoSomething(string fileName) { - using (var stream = File.Open(fileName, FileMode.Open)) - { + using var stream = File.Open(fileName, FileMode.Open); var result = new byte[stream.Length]; + stream.Read(result, 0, (int)stream.Length); // Noncompliant - // ... do something with result - } + + return result; } ---- -=== Compliant solution +==== Compliant solution -[source,csharp] +[source,csharp,diff-id=1,diff-type=compliant] ---- -public void DoSomething(string fileName) +byte[] DoSomething(string fileName) { - using (var stream = File.Open(fileName, FileMode.Open)) - { + using var stream = File.Open(fileName, FileMode.Open); + using var ms = new MemoryStream(); var buffer = new byte[1024]; - using (var ms = new MemoryStream()) + int read; + + while ((read = stream.Read(buffer, 0, buffer.Length)) > 0) { - int read; - while ((read = stream.Read(buffer, 0, buffer.Length)) > 0) - { - ms.Write(buffer, 0, read); - } - // ... do something with ms - } - } + ms.Write(buffer, 0, read); + } + + return ms.ToArray(); } ---- +== Resources + +=== Documentation + +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.read[Stream.Read Method] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync[Stream.ReadAsync Method] + ifdef::env-github,rspecator-view[] From 6f08d3c0ff58b4d2ecbbd9cf4f68c3a5c076570e Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 7 Aug 2024 08:54:11 +0200 Subject: [PATCH 2/2] Address comment + polishing --- rules/S2674/csharp/rule.adoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rules/S2674/csharp/rule.adoc b/rules/S2674/csharp/rule.adoc index d9b9d428372..86ee6bf44e3 100644 --- a/rules/S2674/csharp/rule.adoc +++ b/rules/S2674/csharp/rule.adoc @@ -1,6 +1,6 @@ == Why is this an issue? -Invoking a stream reading method without verifying the number of bytes read can lead to erroneous assumptions. A Stream can represent any I/O operation, reading a file, network communication or inter-process communication, as such it is not guaranteed that the `byte[]` passed into the method will be filled with the requested number of bytes. Therefore, it is crucial to inspect the value returned by the read method to ascertain the actual number of bytes read. +Invoking a stream reading method without verifying the number of bytes read can lead to erroneous assumptions. A Stream can represent any I/O operation, such as reading a file, network communication, or inter-process communication. As such, it is not guaranteed that the `byte[]` passed into the method will be filled with the requested number of bytes. Therefore, inspecting the value returned by the reading method is important to ensure the number of bytes read. Neglecting the returned length read can result in a bug that is difficult to reproduce. @@ -12,7 +12,7 @@ This rule raises an issue when the returned value is ignored for the following m == How to fix it -Ensure to check the return value of `Stream.Read` methods to verify the actual number of bytes read, and use this value when processing the data to avoid potential bugs. +Check the return value of stream reading methods to verify the actual number of bytes read, and use this value when processing the data to avoid potential bugs. === Code examples @@ -20,7 +20,7 @@ Ensure to check the return value of `Stream.Read` methods to verify the actual n [source,csharp,diff-id=1,diff-type=noncompliant] ---- -public byte[] DoSomething(string fileName) +public byte[] ReadFile(string fileName) { using var stream = File.Open(fileName, FileMode.Open); var result = new byte[stream.Length]; @@ -35,7 +35,7 @@ public byte[] DoSomething(string fileName) [source,csharp,diff-id=1,diff-type=compliant] ---- -byte[] DoSomething(string fileName) +public byte[] ReadFile(string fileName) { using var stream = File.Open(fileName, FileMode.Open); using var ms = new MemoryStream();