Skip to content

Add text to OSSL_PARAM constructors#9303

Closed
levitte wants to merge 5 commits intoopenssl:masterfrom
levitte:add-ossl_param_from_text
Closed

Add text to OSSL_PARAM constructors#9303
levitte wants to merge 5 commits intoopenssl:masterfrom
levitte:add-ossl_param_from_text

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jul 3, 2019

This adds OSSL_PARAM_construct_from_text() and OSSL_PARAM_allocate_from_text()

These are utility functions that can be used to replace calls to
ctrl_str type functions with get_params / set_params types of calls.
They work by translating text values to something more suitable for
OSSL_PARAM, and by interpretting parameter keys in a compatible
fashion.

@levitte levitte requested review from mattcaswell and paulidale July 3, 2019 16:48
@levitte
Copy link
Member Author

levitte commented Jul 3, 2019

Note that these are essentially EVP_ctrl_str_to_param() from #8877, and I plan to change #8877 accordingly.

@paulidale
Copy link
Contributor

Is any of #9305 relevant here?

@levitte
Copy link
Member Author

levitte commented Jul 4, 2019

Is any of #9305 relevant here?

Not directly, especially if the #9305 code is kept internal. However, variants of this for PARAM_TEMPLATE use could certainly be added.

@levitte
Copy link
Member Author

levitte commented Jul 4, 2019

Kicking the CIs, they didn't catch the last fixup

@levitte levitte closed this Jul 4, 2019
@levitte levitte reopened this Jul 4, 2019
@levitte
Copy link
Member Author

levitte commented Jul 4, 2019

... or I forgot to commit the latest fixup. D'oh..

@levitte levitte force-pushed the add-ossl_param_from_text branch from 3e1d586 to b149ac9 Compare July 31, 2019 05:49
@levitte levitte marked this pull request as ready for review July 31, 2019 06:09
@slontis slontis self-requested a review July 31, 2019 08:40
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

memory leaks on error cases need addressing

@levitte levitte force-pushed the add-ossl_param_from_text branch 2 times, most recently from 2016098 to 1ddb503 Compare July 31, 2019 11:23
@levitte
Copy link
Member Author

levitte commented Jul 31, 2019

Kicking the bots

@levitte levitte closed this Jul 31, 2019
@levitte levitte reopened this Jul 31, 2019
@levitte levitte mentioned this pull request Aug 2, 2019
2 tasks
They do the same thing as OPENSSL_hexstr2buf() and OPENSSL_buf2hexstr(),
except they take a result buffer from the caller.

We take the opportunity to break out the documentation of the hex to /
from buffer conversion routines from the OPENSSL_malloc() file to its
own file.  These routines aren't memory allocation routines per se.
These are utility functions that can be used to replace calls to
ctrl_str type functions with get_params / set_params types of calls.
They work by translating text values to something more suitable for
OSSL_PARAM, and by interpretting parameter keys in a compatible
fashion.
@levitte levitte force-pushed the add-ossl_param_from_text branch from 1ddb503 to 9115759 Compare August 11, 2019 08:32
@levitte
Copy link
Member Author

levitte commented Aug 12, 2019

Ping!

@levitte
Copy link
Member Author

levitte commented Aug 12, 2019

.. or perhaps not so ping... I suspect a rebase is in order first

@levitte
Copy link
Member Author

levitte commented Aug 12, 2019

It was a function code clash.... but wait, mkerr.pl still adds new function codes??? @richsalz, is this deliberate?

@levitte
Copy link
Member Author

levitte commented Aug 12, 2019

NOW it seems to build right. So, ping!

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Does this need test cases?

@paulidale
Copy link
Contributor

Are unit tests coming?

@levitte
Copy link
Member Author

levitte commented Aug 12, 2019

Are unit tests coming?

I'm using this in #8877, so they do get tested by using... I was hoping to get away with that ;-)

@paulidale
Copy link
Contributor

It seems to be missing negative tests ... e.g. an odd number of hex digits and non-hex digits.
They should be straightforward to add.

@levitte
Copy link
Member Author

levitte commented Aug 12, 2019

Merged.

82bd7c2 Add OPENSSL_hexstr2buf_ex() and OPENSSL_buf2hexstr_ex()
246a1f3 Add OSSL_PARAM_construct_from_text() and OSSL_PARAM_allocate_from_text()

@levitte levitte closed this Aug 12, 2019
levitte added a commit that referenced this pull request Aug 12, 2019
They do the same thing as OPENSSL_hexstr2buf() and OPENSSL_buf2hexstr(),
except they take a result buffer from the caller.

We take the opportunity to break out the documentation of the hex to /
from buffer conversion routines from the OPENSSL_malloc() file to its
own file.  These routines aren't memory allocation routines per se.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #9303)
levitte added a commit that referenced this pull request Aug 12, 2019
These are utility functions that can be used to replace calls to
ctrl_str type functions with get_params / set_params types of calls.
They work by translating text values to something more suitable for
OSSL_PARAM, and by interpretting parameter keys in a compatible
fashion.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #9303)
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

It was a function code clash.... but wait, mkerr.pl still adds new function codes??? @richsalz, is this deliberate?

Yes it was deliberate to not remove function codes because editing mkerr was too complex to just keep the old codes while not adding new ones. See https://github.com/openssl/openssl/blob/master/util/mkerr.pl#L125

crypto/o_str.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

should explicitly test against '\0'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants