Re: rpki-client: replace deprecated ERR_remove_state
Claudio Jeker wrote: > On Thu, Apr 16, 2020 at 09:45:45AM -0600, Theo de Raadt wrote: > > I don't understand the point of any of this cleanup. The process > > is dying and none of these things maintain external state. > > > > I'm going to call it what it is: stylistically stupid and rigid. > > > > Why? Because it would mean every call to errx() in the program is > > wrong because they don't attempt this, and only this one exit() call > > needs this kind of cleanup. That is simply not plausible. > > Indeed, also I noticed that ERR_remove_thread_state() was deprecated in > OpenSSL 1.1 and so I'm kind of back to square 1 again regarding the use of > deprecated functions. > > All this cleanup before exit(3) is done mostly to please runtime memory > leak checkers. > > Also EVP_cleanup() and ERR_free_strings() are deprecated according to > their man page. There is no documentation for CRYPTO_cleanup_all_ex_data() > but looking at the code it seems to be not important. > > Removing all of them seems to be the best move forward. I'd like to add one more point for others following along in the public: Please don't ever use atexit() for this kind of stuff. On OpenBSD, atexit() has some self-protecting safety features, but those don't exist on other systems. As many, once a program does one atexit() call, if a bug is found to corrupt the atexit memory chain in libc, nasty stuff can be done. People should treat atexit like a loaded gun.
Re: rpki-client: replace deprecated ERR_remove_state
Claudio Jeker wrote: > On Thu, Apr 16, 2020 at 09:45:45AM -0600, Theo de Raadt wrote: > > I don't understand the point of any of this cleanup. The process > > is dying and none of these things maintain external state. > > > > I'm going to call it what it is: stylistically stupid and rigid. > > > > Why? Because it would mean every call to errx() in the program is > > wrong because they don't attempt this, and only this one exit() call > > needs this kind of cleanup. That is simply not plausible. > > Indeed, also I noticed that ERR_remove_thread_state() was deprecated in > OpenSSL 1.1 and so I'm kind of back to square 1 again regarding the use of > deprecated functions. > > All this cleanup before exit(3) is done mostly to please runtime memory > leak checkers. Which suggests people aren't actually fuzzing the programs via the errx() paths? I don't believe it. I don't think this was done to please a runtime memory leak checker, I think it was done proactively as a "style choice" in the belief it will please a runtime memory leak checker. So it is waiting for the plane to drop the box in exactly the same place as last time.
Re: rpki-client: replace deprecated ERR_remove_state
On Thu, Apr 16, 2020 at 09:45:45AM -0600, Theo de Raadt wrote: > I don't understand the point of any of this cleanup. The process > is dying and none of these things maintain external state. > > I'm going to call it what it is: stylistically stupid and rigid. > > Why? Because it would mean every call to errx() in the program is > wrong because they don't attempt this, and only this one exit() call > needs this kind of cleanup. That is simply not plausible. Indeed, also I noticed that ERR_remove_thread_state() was deprecated in OpenSSL 1.1 and so I'm kind of back to square 1 again regarding the use of deprecated functions. All this cleanup before exit(3) is done mostly to please runtime memory leak checkers. Also EVP_cleanup() and ERR_free_strings() are deprecated according to their man page. There is no documentation for CRYPTO_cleanup_all_ex_data() but looking at the code it seems to be not important. Removing all of them seems to be the best move forward. > Claudio Jeker wrote: > > > ERR_remove_state(0) is deprecated use the new api > > ERR_remove_thread_state(NULL) instead. > > > > -- > > :wq Claudio > > > > Index: main.c > > === > > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v > > retrieving revision 1.61 > > diff -u -p -r1.61 main.c > > --- main.c 1 Apr 2020 14:15:49 - 1.61 > > +++ main.c 16 Apr 2020 10:01:56 - > > @@ -1250,7 +1250,7 @@ out: > > > > EVP_cleanup(); > > CRYPTO_cleanup_all_ex_data(); > > - ERR_remove_state(0); > > + ERR_remove_thread_state(NULL); > > ERR_free_strings(); > > > > exit(rc); > > > -- :wq Claudio
Re: rpki-client: replace deprecated ERR_remove_state
I don't understand the point of any of this cleanup. The process is dying and none of these things maintain external state. I'm going to call it what it is: stylistically stupid and rigid. Why? Because it would mean every call to errx() in the program is wrong because they don't attempt this, and only this one exit() call needs this kind of cleanup. That is simply not plausible. Claudio Jeker wrote: > ERR_remove_state(0) is deprecated use the new api > ERR_remove_thread_state(NULL) instead. > > -- > :wq Claudio > > Index: main.c > === > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v > retrieving revision 1.61 > diff -u -p -r1.61 main.c > --- main.c1 Apr 2020 14:15:49 - 1.61 > +++ main.c16 Apr 2020 10:01:56 - > @@ -1250,7 +1250,7 @@ out: > > EVP_cleanup(); > CRYPTO_cleanup_all_ex_data(); > - ERR_remove_state(0); > + ERR_remove_thread_state(NULL); > ERR_free_strings(); > > exit(rc); >
rpki-client: replace deprecated ERR_remove_state
ERR_remove_state(0) is deprecated use the new api ERR_remove_thread_state(NULL) instead. -- :wq Claudio Index: main.c === RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v retrieving revision 1.61 diff -u -p -r1.61 main.c --- main.c 1 Apr 2020 14:15:49 - 1.61 +++ main.c 16 Apr 2020 10:01:56 - @@ -1250,7 +1250,7 @@ out: EVP_cleanup(); CRYPTO_cleanup_all_ex_data(); - ERR_remove_state(0); + ERR_remove_thread_state(NULL); ERR_free_strings(); exit(rc);
