Am 07.04.20 um 03:28 schrieb WGH: > I think there has been some misunderstanding about the error handling in my > patch. > > On 4/2/20 5:25 PM, Arne Schwabe wrote: >>> backend_tls_ctx_reload_crl doesn't return an error (as it's void), and its >>> caller never checks OpenSSL error stack. So as this function is, I think it >>> should handle all errors itself, and leave the error stack clear. >> Yes. But your patch throws that error code away. If there an error that >> is not PEM_R_NO_START_LINE it will be silenty discarded. That is my >> problem with using ERR_get_error there. > Please look again: the patch doesn't throw it away, it's printed with > ERR_error_string_n/msg combo. >>> Compare with how it was in current edition of the patch: >>> >>> Thu Apr 2 17:07:13 2020 Diffie-Hellman initialized with 2048 bit key >>> Thu Apr 2 17:07:13 2020 OpenSSL: error:0909006C:PEM >>> routines:get_name:no start line >>> Thu Apr 2 17:07:13 2020 CRL: cannot read CRL from file >>> combined_crl.pem: error:0908F066:PEM routines:get_header_and_data:bad end >>> line >>> Thu Apr 2 17:07:13 2020 CRL: loaded 1 CRLs from file combined_crl.pem >>> >> Yeah but *always* having a scary error that translates to "No more crls >> found in the file" is not very user friendly since most people will not >> understand that this is to be expected. > > Perhaps you glanced over and didn't notice that it says "bad _end_ line"? > Here I simulated a corrupt file, causing this error, to highlight the > difference in how errors are printed by crypto_msg and by my ad-hoc code. My > code does a better job at indicating that the error comes from the CRL file, > where messages printed by crypto_msg may blend with other bogus errors and > warnings, like it happened with that bogus warning you fixed in the > aforementioned patch. >
Yeah sorry, the formatting of the patch made me believe the control flow was different from what it really is. But this bogus error message was a one off mistake in OpenVPN. In general printing an error before the message isn't a big problem. > Also, my patch doesn't print "no start line" error unless there were no valid > CRLs inside the specified file. Printing a scary error in the latter case is > IMHO justfified: file is either empty due to some administrative mistake > (e.g. buggy updating scripts) or corrupt. > > I do not insist on leaving my logging in place, though, and will replace it > with crypto_msg as you suggested. After all, crypto_msg is used all over the > place, and making its errors more understandable is something out of scope of > this patch. > I feel consistency in code is important. And crypto_msg will also print the whole error stack instead of just the last message (which probably does not matter in this case). Since crypto_msg is used all over the place, its printing should be improved instead of having different error message handling/printing all over the place. For your one-off patch it might seem superior but for maintaining the code it is not. Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
