Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BN_pseudo_rand is really BN_rand #3743

Closed
wants to merge 1 commit into from
Closed

BN_pseudo_rand is really BN_rand #3743

wants to merge 1 commit into from

Conversation

richsalz
Copy link
Contributor

And BN_pseudo_rand_range is really BN_rand_range

I am starting to look at updating the RAND family for this release, and I started looking at bn_rand. It turns out that the "psuedo" BN rand functions don't do anything different. So I renamed a parameter to be more clear, and replaced BN_pseudo_rand{_range} with BN_rand{_range}

@richsalz richsalz added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Jun 21, 2017
@richsalz richsalz self-assigned this Jun 21, 2017
@richsalz
Copy link
Contributor Author

Added a commit which deprecates them.

int BN_pseudo_rand_range(BIGNUM *rnd, const BIGNUM *range);
DEPRECATEDIN_1_1_0(int BN_pseudo_rand(BIGNUM *rnd,
int bits, int top, int bottom))
DEPRECATEDIN_1_1_0(int BN_pseudo_rand_range(BIGNUM *rnd, const BIGNUM *range))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ok, this is a change of an existing API. You may, however, use DEPRECATEDIN_1_2_0, it just needs to be defined in include/openssl/opensslconf.h.in (I'm surprised, I thought that was already added... by @mattcaswell if I remember correctly)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm surprised, I thought that was already added... by @mattcaswell if I remember correctly)

It was in #3544 which never got merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. So @richsalz, this is the commit to snatch: 2597a2e

int BN_rand_range(BIGNUM *r, const BIGNUM *range)
{
return bn_rand_range(0, r, range);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errr, so where is it implemented? It's still declared in bn.h!

{
return bn_rand_range(0, r, range);
}

int BN_pseudo_rand_range(BIGNUM *r, const BIGNUM *range)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function needs to be guarded with:

#if (OPENSSL_API_COMPAT < 0x10200000L)

@@ -100,19 +100,17 @@ int BN_rand(BIGNUM *rnd, int bits, int top, int bottom)

int BN_pseudo_rand(BIGNUM *rnd, int bits, int top, int bottom)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function needs to be guarded with:

#if (OPENSSL_API_COMPAT < 0x10200000L)

@richsalz
Copy link
Contributor Author

updated the second commit to do the deprecation right for 1.2.0
The old static bn_rand_range was renamed to BN_rand_range
Addressed all issues so far.

# define DEPRECATEDIN_1_2_0(f) DECLARE_DEPRECATED(f)
#else
# define DEPRECATEDIN_1_2_0(f)
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to be missing something. We already have variables marked as deprecated in 1.2.0, but I can't find how.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using OPENSSL_API_COMPAT, e.g.

# if OPENSSL_API_COMPAT < 0x10200000L
/*
 * LONG and ZLONG are strongly discouraged for use as stored data, as the
 * underlying C type (long) differs in size depending on the architecture.
 * They are designed with 32-bit longs in mind.
 */
DECLARE_ASN1_ITEM(LONG)
DECLARE_ASN1_ITEM(ZLONG)
# endif

@richsalz
Copy link
Contributor Author

never mind prev comment

@richsalz
Copy link
Contributor Author

richsalz commented Jul 3, 2017

ping. we shouldn't be calling deprecated functions inside the library :)

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, seems my approval was premature. Travis failure is relevant.

@richsalz
Copy link
Contributor Author

richsalz commented Jul 3, 2017 via email

@richsalz
Copy link
Contributor Author

richsalz commented Jul 3, 2017

Build failures are unrelated. I will to nudge them again.

@levitte
Copy link
Member

levitte commented Jul 3, 2017

A rebase on a fresher master might help

@richsalz
Copy link
Contributor Author

richsalz commented Jul 3, 2017 via email

@richsalz
Copy link
Contributor Author

richsalz commented Jul 3, 2017

builds clean, ping @mattcaswell

And BN_pseudo_rand_range is really BN_rand_range.
Document that we might deprecate those functions.
levitte pushed a commit that referenced this pull request Jul 3, 2017
And BN_pseudo_rand_range is really BN_rand_range.
Document that we might deprecate those functions.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #3743)
@richsalz
Copy link
Contributor Author

richsalz commented Jul 3, 2017

thanks.

@richsalz richsalz closed this Jul 3, 2017
@richsalz richsalz deleted the rm-pseudo-rand branch July 3, 2017 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: review pending This pull request needs review by a committer branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants