Missing giterr_set before returning an error in stransport.c + git error conventions for wrappers?

[alexcrichton]

Hello! We've recently been seeing some weird error messages on OSX from Cargo along the lines of:

$ cargo install --git https://wrong.host.badssl.com
    Updating git repository `https://wrong.host.badssl.com`
error: failed to clone into: /Users/acrichton/.cargo/git/db/_empty-90dfa5825

Caused by:
  [7/-17] config value 'credential.helper' was not found

Oddly though on Linux the error is different!

$ cargo install --git https://wrong.host.badssl.com
    Updating git repository `https://wrong.host.badssl.com/`
error: failed to fetch into /home/alex/.cargo/git/db/_empty-90dfa5825b276e27

Caused by:
  [16/-17] hostname does not match certificate

The Linux behavior is what I'd expect to happen but the OSX behavior is also interesting! Now clearly this error message (and Cargo) is pretty far removed from libgit2, but I think I've managed to trace the bug back to libgit2.

On OSX one piece of the error is -17 which is the code of the error and corresponds to GIT_ECERTIFICATE which is what I'd expect! Unfortunately though the error message seems to be incorrect (other than the code). I think that the error message itself originally came from git2-rs, the Rust bindings for libgit2 specifically at one point where it loads credential.helper and presumably fails (but handles the error locally).

Stepping through in a debugger I found that the GIT_ECERTIFICATE error originates from this location where notably there is no invocation of giterr_set. When I print out the contents of giterr_last at that location it sure enough mentions the credential helper business. Also sure enough in openssl.c when GIT_ECERTIFICATE is returned a giterr_set happens!

So I think most of this just boils down to perhaps a missing call to giterr_set in the stransport.c bindings? Or maybe there's something else that should be configuring that message?

---

I'd also like to, while I can, ask a more general question as well. I don't think this is the first time we've run across reporting "stale errors" in that the error from one operation persists to accidentally get returned from a different operation. Is this normal and/or expected behavior from libgit2? Should wrappers like git2-rs always clear the error before any operation is invoked in libgit2?

[alexcrichton]

Note that I've also been sort of hazy before on the distinction between error codes and error classes in libgit2 so I could also just be totally off-base in terms of how git2-rs interprets errors!

[ethomson]

So I think most of this just boils down to perhaps a missing call to giterr_set in the stransport.c bindings?

Yes, your analysis is correct - we should be setting the error message.

I'd also like to, while I can, ask a more general question as well. I don't think this is the first time we've run across reporting "stale errors" in that the error from one operation persists to accidentally get returned from a different operation. Is this normal and/or expected behavior from libgit2? Should wrappers like git2-rs always clear the error before any operation is invoked in libgit2?

No, this is definitely an error in libgit2. We should always be setting error messages correctly. If you get a -1 back from libgit2, there should be a good error message. If you're seeing stale errors in this case, please report them. However, we do not clear error messages on success. So if you get a success back then you will see random stale error messages.

Note that I've also been sort of hazy before on the distinction between error codes and error classes in libgit2 so I could also just be totally off-base in terms of how git2-rs interprets errors!

Error codes should be programmatically actionable. Success, general failure, or a specific failure that allows you to handle the error in a particular way. (eg, -1 is basically just a fatal error, while GIT_EAGAIN is a more descriptive code that indicates how you can handle it, perhaps by retrying.)

Error classes are basically just semantic sugar on top to tell you broadly what region the error occurred in. You might want to throw an ObjectDatabaseException or something based on the error class. Maybe you would customize an end-user error message based on this (eg, "An error occurred in the object database: object deadbeef is corrupt.") But it's not suuuper likely that you would really be able to do something programmatic with the class.

Thanks for the report, and I hope this explanation helps.

[alexcrichton]

Awesome thanks for the info @ethomson! That's definitely quite helpful. If this comes up again I may just skip straight to the PR :)

I hope this explanation helps

That makes total sense! I think I might just incorporate some of that into the git2-rs documentation...