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

2020-02-10 Thread Bernd Edlinger
On 2/10/20 6:29 PM, Kurt Roeckx wrote:
> On Mon, Feb 10, 2020 at 04:19:20PM +, Matt Caswell wrote:
>>
>>
>> On 10/02/2020 00:15, SHANE LONTIS wrote:
>>> 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?
>>
>> Yes, I think we should be.
> 
> I think that as long as we have that fallback code, that it should
> be checked.
> 
> 

Yes, although I wonder why a function that checks
the return value of CRYPTO_THREAD_write_lock and
CRYPTO_THREAD_unlock does not check for
possible overflow of the addition, which is
far more likely to happen.


Bernd.


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

2020-02-10 Thread Kurt Roeckx
On Mon, Feb 10, 2020 at 04:19:20PM +, Matt Caswell wrote:
> 
> 
> On 10/02/2020 00:15, SHANE LONTIS wrote:
> > 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?
> 
> Yes, I think we should be.

I think that as long as we have that fallback code, that it should
be checked.


Kurt



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

2020-02-10 Thread Matt Caswell



On 10/02/2020 00:15, SHANE LONTIS wrote:
> 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?

Yes, I think we should be.

Matt

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

2020-02-10 Thread Sam Roberts
I've seen some projects enjoying https://github.com/apps/stale, though
its focus is not so much relabelling things that are ready, but
closing things that are not active, which might not be relevant here.

On Sun, Feb 9, 2020 at 2:23 AM Mark J Cox  wrote:
>
> 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