Re: [libvirt] [PATCH 1/5] virauth.c: Check for valid auth callback

On 08/02/2018 08:27 PM, Marcos Paulo de Souza wrote: I believe the virAuthGet{Username|Password}Path API's should be adjusted instead. Although not possible today as shown by the subsequent patches, if @auth or !auth->cb were NULL calling virAuthGetCredential and returning something is still possible since neither auth nor auth->cb is referenced. They are only referenced when it's required to ask via some mechanism. It's at those points we should generate the errors. Furthermore, the callers that generate their own errors based on where in the code they fail; however, those errors would hide memory allocation failures from say virAsprintf or VIR_STRDUP calls from other virAuth* APIs that are called. For example, calling virAuthGetConfigFilePathURI and failing to allocate memory would generate a memory allocation failure; however, the caller could overwrite that with a "{Username|Password} request failed" error message. Interestingly, the virAuthGetUsernamePath caller expects the error to be generated and doesn't generate it's own, but virAuthGetPasswordPath callers will generate (and overwrite) their own error. So, I think all of the error generation can be done in the *Path API's and a lot more cleanup is possible... First, just before the memset(&cred) (in both UsernamePath and PasswordPath) add the logic that would check and error for "if (!auth) {" with an error message such as "Missing authentication credentials" using VIR_ERR_INVALID_ARG for virReportError and return NULL. Next, checking and erroring for !auth->cb would only be necessary if we ever get past the credtype "continue;" condition. In that case the error message would be "Missing authentication callback" using VIR_ERR_INVALID_ARG for virReportError and return NULL. With those errors in place, there would be two more reasons that the caller would need to generate an error... First if there was no expected credtype for the API or if the (*cred->cb) returns < 0. Since the for loop breaks once the auth->cb is called, if we change the "break;" to a return cred.result and then instead of the bare return cred.result at the bottom we turn that into an error "Missing XXX credential type" (where XXX would be replaced by "VIR_CRED_AUTHNAME" and "VIR_CRED_PASSPHRASE or VIR_CRED_NOECHOPROMPT" using VIR_ERR_AUTH_FAILED for virReportError followed by return NULL. With that in place, add error processing to the auth->cb, either: virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); or virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); when (*auth->cb) < 0 Once the above is all in place, then the callers could be adjusted to not generate any errors for !auth, !auth->cb, and NULL return from the function. Take the opportunity to clean up the callers a bit to alter long lines and the calls to be just: if (!(xxxx = virAuthGet*(...))) goto xxxx; where the virAuthGet* call could be across multiple lines if it goes beyond the 80 cols, but the open/close parens {} aren't needed. The commit messages for the subsequent patches would need to be adjusted to note that you're removing the need to generate error messages for the virAuthGet{Username|Password}[Path] callers since the API's handle that. The test_driver and rpc/virnet*session.c callers do have different messages on NULL failures now, but (famous last words) that shouldn't be a problem. NB: The rpc/virnet[lib]sshsession.c consumers already do the !auth || !auth->cb checks in the form of "if (!sess->cred || !sess->cred->cb) {". I would just keep those as is. Finally, not sure how it's called, but xenapiUtil_RequestPassword would perhaps need similar adjustments. If it's caller still overwrites the message, then at least the message is logged in a domain log file somewhere. Hopefully this makes sense. This one patch is blossoming into many more and the subsequent patches get a bit more code removal using the common error messages. John Could have taken the opportunity to fix the indention issue caused by commit '12614e7e2' (e.g. s/defaultUsername/ defaultUsername/), but with the above suggestions that would need a separate/unnecessary patch. We'll just have to wait until someone comes this way again.