GNOME Bugzilla – Bug 780310
g_tls_database_verify_chain doesn't set the GError for failures other than cancellation
Last modified: 2017-04-03 16:58:42 UTC
It is idiomatic for GIO API to set the GError *and* return an appropriate return value when there is a failure. Sometimes there are exceptions and deviations (eg., the libsecret and gcr API), but I think those should be clearly documented. Currently g_tls_database_verify_chain throws an error only if the operation was cancelled. If the certificate could not be verified due to actual problems with it, no error is thrown and only the flags (ie. the return value) is set. This can trick code that follows this pattern: error = NULL; result = foo_operation (arg1, ..., &error); if (error != NULL) { /* handle the error */ } We should either throw an error or document this.
Throwing an error would be an API break, so this would be a documentation thing. I guess the problem is that the function's name is wrong; it's not "verify that the chain is valid", it's "determine the verified-ness of the chain". Determining that the chain is invalid is considered success; it only returns an error if something prevents it from being able to check the chain at all.
Created attachment 348524 [details] [review] docs: Clarify the use of the GError in g_tls_database_verify_chain*
Comment on attachment 348524 [details] [review] docs: Clarify the use of the GError in g_tls_database_verify_chain* That wording is kind of confusing. I'd suggest something like: If @chain is found to be valid, then the return value will be 0. If @chain is found to be invalid, then the return value will indicate the problems found. If the function is unable to determine whether @chain is valid or not (eg, because @cancellable is triggered before it completes) then the return value will be %G_TLS_CERTIFICATE_GENERIC_ERROR and @error will be set accordingly. (If you want to add an additional parenthetical "(@error is not set when @chain is successfully analyzed but found to be invalid)" then go ahead.)
Created attachment 349174 [details] [review] docs: Clarify the use of the GError in g_tls_database_verify_chain*
(In reply to Dan Winship from comment #3) Thanks for the review! > That wording is kind of confusing. I'd suggest something like: > > If @chain is found to be valid, then the return value will be 0. If > @chain is found to be invalid, then the return value will indicate the > problems found. If the function is unable to determine whether @chain > is valid or not (eg, because @cancellable is triggered before it > completes) then the return value will be %G_TLS_CERTIFICATE_GENERIC_ERROR > and @error will be set accordingly. > > (If you want to add an additional parenthetical "(@error is not set when > @chain is successfully analyzed but found to be invalid)" then go ahead.) I updated the wording. What do you think?
Comment on attachment 349174 [details] [review] docs: Clarify the use of the GError in g_tls_database_verify_chain* Pushed to master. Thanks.