Should the return result of CRYPTO_UP_REF() / CRYPTO_DOWN_REF() be checked?

[SHANE LONTIS]

With the new architecture changes there are quite a few new calls to

CRYPTO_UP_REF()
CRYPTO_DOWN_REF()

These methods return an int that is not being checked in lots of places.

This return value only seems to affect fallback code that calls 
CRYPTO_atomic_add (which can return 0 on lock or unlock failure)

SO the question is should we be checking this return value?


Note that not checking has resulted in a few assumptions in other code…
e.g the following function returns void.
 
/crypto/evp/keymgmt_lib.c: 165 in evp_keymgmt_util_cache_pkey()
159 }
160 
161 void evp_keymgmt_util_cache_pkey(EVP_PKEY *pk, size_t index,
162  EVP_KEYMGMT *keymgmt, void *keydata)
163 {
164 if (keydata != NULL) {
>>>CID 1458170:  Error handling issues  (CHECKED_RETURN)
>>>Calling "EVP_KEYMGMT_up_ref" without checking return value (as is done 
>>> elsewhere 4 out of 5 times).
165 EVP_KEYMGMT_up_ref(keymgmt);

NOTE: EVP_KEYMGMT_up_ref() just does an CRYPTO_UP_REF() call and always returns 
1.

Re: Github PR label automation

[Mark J Cox]

Okay, I'll leave things as they are with those issues and just add the
addition of notification on urgent labels.

As to the earlier question form the thread as why this isn't a github
automation: the actual reason I was writing the script was to get some
on-demand statistics for my own interest and it just turned out this
was something that bugged me that I could use the same framework for
(I did a trivial review and thought how annoying it would be to have
to manually set a reminder myself to do a future action).  I was a
pretty trivial (few hours) script, so I have no concern if it is done
differently or not used in the future should a better way present
itself.

Mark

On Sun, Feb 9, 2020 at 12:19 AM Matt Caswell  wrote:
>
>
>
> On 08/02/2020 15:56, Mark J Cox wrote:
> > I've currently got a cron job running every hour that looks at open PR
> > requests against github openssl repo and does various actions.  So if
> > you were wondering why I was altering labels and making comments at
> > 4am, now you know.  No doubt we'll use some tool user for this in the
> > future.
> >
> > So right now here's what it does:
> >
> > Every hour it looks at open PRs that are labelled "approval: done".
> > If 24 hours has elapsed since that label was assigned and if there
> > have been no comments made to the PR since the label was assigned then
> > it is automatically moved to "approval: ready to merge" with a comment
> > added to trigger notifications.  So if you want to stop something
> > going to "ready to merge" just add any comment to the PR.
> >
> > I'm thinking of using this script also to 1) collect interesting stats
> > and 2) do some other actions.  So if there's some automation you'd
> > like to see just add an enhancement issue against the openssl/tools
> > repo.
> >
> > 1 Matt already asked for committer notification trigger for anything
> > labelled Urgent.
> >
> > 2 If there were comments made after "approval: done" then I think we
> > really ought to drop the "approval: done" label as the comments likely
> > invalidated the approval.  So I'll likely add that next week (if
> > "approval: done" label and has comments since that label then remove
> > the label and add a comment 'please review if this is really approval:
> > done'.  If the approval: done label gets set again then after 24 hours
> > the existing automation will trigger.  #10786 is a good example of
> > this.
>
> I'm less sure about this. I think there are probably many examples where
> this is not the case. E.g. a quick review found these recent cases:
>
> https://github.com/openssl/openssl/pull/11003
>
> https://github.com/openssl/openssl/pull/11015
>
> https://github.com/openssl/openssl/pull/10992
>
> https://github.com/openssl/openssl/pull/10991
>
> https://github.com/openssl/openssl/pull/10990
>
> https://github.com/openssl/openssl/pull/10989
>
> I would be more minded to think that if a comment invalidates an
> "approval done" then the committer should explicitly decide to do that.
>
> Matt