pppd: add error message getter for openssl #508
Conversation
And use this for digest functions in case of error. Signed-off-by: Frederic Martinsons <[email protected]>
|
See #496 for context. Note that I added this error printing only for EVP_DigestInit call , there may be other places where this may be interesting but since I don't know the project, I preferred to kept this minimal |
| @@ -179,6 +180,22 @@ int PPP_crypto_deinit() | |||
| return 1; | |||
| } | |||
|
|
|||
| char *PPP_crypto_get_error() | |||
There was a problem hiding this comment.
This will work, and generally I think it can be committed with further ado. However, you could consider
- Avoid multi-line errors and maybe get the last error at the top of the stack?
- Change this to a function signature such that it takes a format string, adds the error information and calls error().. e.g. PPP_ERR_printf("chap failed, md4 initialization failed");
- All these heap allocations bugs me when memory may not need to be allocated, could consider making err_buf[256] a part of PPP_MD_CTX or PPP_CHIPHER_CTX and then use that with an snprintf. Yes, an imposed limit isn't ideal, but it would prevent it from spewing out multi-line information in the logs.
In the secondary example, we could for every if-else situation where the crypto api is called add the code to improve current status quo, instead of just the init functions. If we make it a part of the shim API, then the caller won't need to add the if-else, but you loose any contextual information such as e.g. ms-chap-crypto failed, md4 algorithm couldn't be initialized.
Yes, openssl has a similar get_error() and get_error_string() functions, and for this to work you also need to load the strings, e.g. SSL_load_error_strings, unless the BIO api doesn't require this.
To make this a complete change, perhaps don't do the error handling in the individual md4/md5/sha1 files, but move it one level up to the PPP_DigestInit(), PPP_CipherInit() and friends.
There was a problem hiding this comment.
Thank you very much for your comments, if I understand correctly
- change mutli line by last error
- change the error API to get a format string as input to prepend to openssl error
- avoid heap allocations (this one bugs me since I will not know the size of error string in advance and it may be cut by the snprintf that we will do)
- move the error management in the PPP crypto abstraction layer.
I'll work on that as soon as I have free time
|
@fmartinsons Just did in #509 |
|
Ok , I'll close this one then, thanks. |
And use this for digest functions in case of error.