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

Improper Resource Shutdown or Release #381

Open
Andrew1178 opened this issue May 5, 2018 · 2 comments
Open

Improper Resource Shutdown or Release #381

Andrew1178 opened this issue May 5, 2018 · 2 comments

Comments

@Andrew1178
Copy link

Really small but this was highlighted in a veracode scan of ours.

In RaygunRequestMessageBuilder.cs line 114 you guys create a new instance of StreamReader and don'
t dispose of it. Can you please wrap it in a using? Or in your try catch dispose of it?

Thanks

https://github.com/MindscapeHQ/raygun4net/blob/master/Mindscape.Raygun4Net/Builders/RaygunRequestMessageBuilder.cs

@xt0rted
Copy link

xt0rted commented May 5, 2018

Calling Dispose() on the StreamReader will also dispose the underlying stream, in this case request.InputStream. It's not raygun's responsibility to dispose of that since it didn't create it (the framework did, and it will dispose of it at the end of the request).

If you dispose of the underlying stream then any caller after raygun won't have access to it. There's a constructor overload that takes a bool indicating if you want to leave the stream open when calling dispose, but that achieves the same result as what the code does currently.

This is the Dispose() method of the StreamReader class

protected override void Dispose(bool disposing)
{
    // Dispose of our resources if this StreamReader is closable.
    // Note that Console.In should be left open.
    try {
        // Note that Stream.Close() can potentially throw here. So we need to 
        // ensure cleaning up internal resources, inside the finally block.  
        if (!LeaveOpen && disposing && (stream != null))
            stream.Close();
    }
    finally {
        if (!LeaveOpen && (stream != null)) {
            stream = null;
            encoding = null;
            decoder = null;
            byteBuffer = null;
            charBuffer = null;
            charPos = 0;
            charLen = 0;
            base.Dispose(disposing);
        }
    }
}

@QuantumNightmare
Copy link
Contributor

Thanks, we could look at using the constructor overload or perhaps using Stream.Read instead of creating a new reader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants