On Fri, 2022-11-04 at 01:35 +0300, Vitaly Chikunov wrote:
> On Fri, Nov 04, 2022 at 01:24:21AM +0300, Vitaly Chikunov wrote:
> > On Fri, Nov 04, 2022 at 01:05:31AM +0300, Vitaly Chikunov wrote:
> > > Mimi,
> > >
> > > On Thu, Nov 03, 2022 at 02:38:49PM -0400, Mimi Zohar wrote:
> > > > Define a log_errno_reset macro to emit the errno string at or near the
> > > > time of error, similar to the existing log_errno macro, but also reset
> > > > errno to avoid dangling or duplicate errno messages on exit.
> > > >
> > > > The initial usage is for non-critical file open failures.
> > > >
> > > > Suggested-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
> > > > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > > > ---
> > > > src/evmctl.c | 12 ++++++++++--
> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > Reviewed-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
Thanks!
> > >
> > > >
> > > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > > index 0412bc0ac2b0..54123bf20f03 100644
> > > > --- a/src/evmctl.c
> > > > +++ b/src/evmctl.c
> > > > @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file)
> > > > int err_padded = -1;
> > > > int err = -1;
> > > >
> > > > - errno = 0;
> > > > memset(zero, 0, MAX_DIGEST_SIZE);
> > > >
> > > > pseudo_padded_banks = init_tpm_banks(&num_banks);
> > > > @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file)
> > > > init_public_keys(imaevm_params.keyfile);
> > > > else /* assume read pubkey from x509 cert */
> > > > init_public_keys("/etc/keys/x509_evm.der");
> > > > + if (errno)
> > > > + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys");
> > >
> > > Library prints appropriate error messages, so this is perhaps just to
> > > clear errno. But it's not necessarily completely failed, but maybe
> > > failure in one key. So I would say "Failure in initializing public
> > > keys" to be precise.
> > >
> > > ps.
> > >
> > > BTW, init_public_keys API call cannot return error except by errno,
> > > but it does not set it consistently so some errors may be missed.
> > >
> > > init_public_keys loops calling read_pub_pkey
> > >
> > > entry->key = read_pub_pkey(keyfile, 1);
> > > if (!entry->key) {
> > > free(entry);
> > > continue;
> > > }
> > >
> > > and read_pub_pkey have such code:
> > >
> > > if (!keyfile)
> > > return NULL;
> > >
> > > In that case some key is not read but we don't get any error notification.
> > >
> > > I think it's legal, by the right of being library, so set `errno =
> > > EINVAL` there somewhere. But, I'm not sure where - as we should not
> > > clobber existing errno values. Perhaps, errno setting should be added to
> > > libimaevm consistently to all functions, but this is huge task, so I
> > > would not suggest to do it now. Just suggestion for the future
> > > developments, maybe.
> >
> > Just to compare with other library - libtracefs sets errno _sometimes_,
>
> It "is not consistent" in the sense that error in the API call does not
> have always errno set. And this is unlike we have for common libc API
> calls where errno is defined. (As a consequence we cannot just add
> strerror() to the printing errors from these APIs).
>
> But, it's not necessarily there is standard or common practice about
> this matter.
The perror() man page seems to say that it is optional, but doesn't say
anything about libraries being self-consistent: "When system call
fails, it usually returns -1 and sets the variable errno to a value
describing what went wrong. (These values can be found in
<errno.h>.) Many library functions do likewise."
>
>
> > for example, their API call tracefs_dynevent_get have:
> >
> > struct tracefs_dynevent *
> > tracefs_dynevent_get(enum tracefs_dynevent_type type, const char *system,
> > const char *event)
> > {
> > ...
> > if (!event) {
> > errno = -EINVAL;
> > return NULL;
> > }
> >
> > count = get_all_dynevents(type, system, &events);
> > if (count <= 0)
> > return NULL;
> > ...
> >
> > So it sets errno sometimes, but not always, which I am not sure is correct
> > approach. This needs to be discussed more with library experts.
Perhaps this isn't a question of self-consistency, but of who is
setting errno. In the tracefs_dynevent_get() case, nothing else is
being called, so nothing could have set errno. In the
get_all_dynevents() case, without looking at the code, something could
have already set errno.
As for the ima-evm-utils example, I agree read_pub_pkey() should return
-EINVAL when keyfile is NULL. init_public_keys() verifies keyfile is
not NULL before calling read_pub_pkey(), so read_pub_pkey() returning
-EINVAL, shouldn't affect it.
--
Thanks,
Mimi
