Skip to content

Fixes Coverity NullReferenceException Issues #1081

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

Merged
merged 2 commits into from
Jun 9, 2015
Merged

Fixes Coverity NullReferenceException Issues #1081

merged 2 commits into from
Jun 9, 2015

Conversation

whoisj
Copy link

@whoisj whoisj commented Jun 8, 2015

The new-again Coverity scanning has started to bear fruit. Here are some of the NRE issues identified by the scan. Minor fixes, low risk. Not sure if I'm throwing the correct exception types here (/CC @nulltoken).

  1. Do not assume disk reads always succeed, check results before processing.
  2. Verify parameters being using.
  3. There's more than one kind of null when dealing with p/invoke and handles.
  4. Better to give specific exceptions than general exceptions when reading in bad data.

@nulltoken
Copy link
Member

@whoisj I think Version.cs is already targeted by #1080

@@ -251,7 +251,7 @@ public virtual Blob CreateBlob(Stream stream, long numberOfBytesToConsume)

using (var odbStream = Proxy.git_odb_open_wstream(handle, numberOfBytesToConsume, GitObjectType.Blob))
{
var buffer = new byte[4*1024];
var buffer = new byte[4 * 1024];
Copy link
Member

Choose a reason for hiding this comment

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

Oh.... Look Ma... Spaces! :trollface:

Copy link
Author

Choose a reason for hiding this comment

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

Spaces are bad? Not sure what you mean here. Is it just that the Roslyn autoformatter struck again?

Copy link
Member

Choose a reason for hiding this comment

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

Is it just that the Roslyn autoformatter struck again?

@whoisj I'm not against spaces 😉 However, I'd rather keep (unrelated) beautification in separate commits.

Copy link
Author

Choose a reason for hiding this comment

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

Ah gotcha - @#$%! Roslyn and her free beautification on every } and ; 😁

@whoisj
Copy link
Author

whoisj commented Jun 8, 2015

@nulltoken fixed up. Removed my fixes for Version.cs

nulltoken added a commit that referenced this pull request Jun 9, 2015
Fixes Coverity NullReferenceException Issues
@nulltoken nulltoken merged commit 738c263 into libgit2:vNext Jun 9, 2015
@nulltoken
Copy link
Member

🎱

@nulltoken nulltoken added this to the v0.22 milestone Jun 9, 2015
@whoisj whoisj deleted the fix-coverity-nre-issues branch June 10, 2015 16:19
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

Successfully merging this pull request may close these issues.

2 participants