Re: [PATCH ima-evm-utils v3 06/15] Add missing EVP_MD_CTX_free() call in calc_evm_hash()

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2022-09-15 at 18:36 +0300, Vitaly Chikunov wrote:
> On Thu, Sep 15, 2022 at 07:58:51AM -0400, Mimi Zohar wrote:
> > Hi Vitaly,
> > 
> > Thank you for this and the other reviews.  They'll be addressed in the
> > next version.
> > 
> > On Wed, 2022-09-14 at 17:51 +0300, Vitaly Chikunov wrote:
> > > > @@ -350,12 +356,11 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > >  #if OPENSSL_VERSION_NUMBER < 0x10100000
> > > >       EVP_MD_CTX ctx;
> > > >       pctx = &ctx;
> > > > -#else
> > > > -     pctx = EVP_MD_CTX_new();
> > > >  #endif
> > > >  
> > > >       if (lstat(file, &st)) {
> > > >               log_err("Failed to stat: %s\n", file);
> > > > +             errno = 0;
> > > 
> > > Why it clears errno (here and below)?
> > > 
> > > errno(3) says "The value of errno is never set to zero by any system
> > > call or library function."
> > 
> > evmctl, itself, is not a system call or a library function. 
> 
> Ah, I wasn't attentive this is only evmctl. [But there's similar commit
> acb19d1 ("Reset 'errno' after failure to open or access a file")
> changing libimaevm.c which is wrong.]
> 
> Perhaps it should be noted in commit message that errno is cleared
> because it's error message is already printed to avoid double printing
> at exit of cmd handler.
> 
> > Is this a
> > generic statement or here in particular as to how evmctl should handle
> > failed system calls?   Another example is reading the keyfile.  The
> > existence of which is not required.
> 
>           log_err("Failed to stat: %s\n", file);
> 
> This does not even output errno code, but it could be very informative
> to user. I think it's better to print (at least errno or) strerror for
> users there (and on other syscall errors log_err instances.
> 
> Maybe to add special log function (like log_strerr) just for evmctl
> which will print (non "\n"-terminated) error message (similar to
> warn(3)) with strerror output appended (and commented in the code why
> it) clears errno (so that later handlers do not print it again).
> 
> ps. About libimaevm.c--I think errno should not be touched there as this
> breaks what coders expect from libraries. If this affects exit of evmctl
> then it should be handled in evmctl, not in the library. (Of course it's
> better to add strerror(errno) to log_err there too, but not by the
> proposed above function.)

Thank you for the suggestions.  log_errno() is already defined.  Not
sure how I missed that.  So use log_errno() in libimaevm.c.  For
evmctl.c, define a wrapper named log_and_reset_errno(), with your
suggested patch description.

Since none of the changes in next-testing have been released, they can
still be fixed/squashed as needed.

thanks!

Mimi