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

[Vitaly Chikunov <vt@altlinux.org>]

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.)

Vitaly,

> 
> thanks,
> 
> Mimi
>