Skip to content

add heap hint support for a few of the x509 functions#7136

Merged
dgarske merged 4 commits intowolfSSL:masterfrom
jpbland1:x509-new-ex
Jan 19, 2024
Merged

add heap hint support for a few of the x509 functions#7136
dgarske merged 4 commits intowolfSSL:masterfrom
jpbland1:x509-new-ex

Conversation

@jpbland1
Copy link
Copy Markdown
Contributor

Description

Adds heap hint support only for the functions the customer asked for

Fixes zd# 17258

Testing

Uses api and crl tests

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@jpbland1 jpbland1 marked this pull request as ready for review January 17, 2024 16:28
Comment thread src/x509_str.c Outdated

/* tmp ctx for setting our cert manager */
ctx = wolfSSL_CTX_new(cm_pick_method());
ctx = wolfSSL_CTX_new(cm_pick_method(NULL));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use str->cm->heap

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah ok, str->cm is checkedfor NULL

Comment thread wolfssl/ssl.h Outdated
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_new(void);
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_new_ex(void* heap);
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_dup(WOLFSSL_X509* x);
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_dup_ex(WOLFSSL_X509* x, void* heap);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you really need wolfSSL_X509_dup_ex? Can't the wolfSSL_X509_dup just use x->heap from the previous one? If you have places where heap isn't populated then switch those to using wolfSSL_X509_new_ex.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you're right, I had this in my other PR but was following what the customer gave me for this one

@jpbland1
Copy link
Copy Markdown
Contributor Author

retest this please

@jpbland1
Copy link
Copy Markdown
Contributor Author

I'm getting:

Got -132 but expected -171 at line 4947 in ecdsa_invalid_data_tests
Encountered an unexpected result
 ECDSA-INVALID ... FAILED

which I don' think I've changed, lets try one more time:
retest this please

@dgarske
Copy link
Copy Markdown
Member

dgarske commented Jan 17, 2024

@jpbland1 . This is a known issue. @lealem47 is working on it.

@jpbland1 jpbland1 requested a review from dgarske January 17, 2024 20:03
@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske Jan 17, 2024
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks

@lealem47
Copy link
Copy Markdown
Contributor

Jenkins Retest this please

Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Please add a test case for wolfSSL_X509_d2i_ex or adapt an existing one. How have you confirmed all X509 nodes have the heap hint? Did you try with WOLFSSL_HEAP_TEST?

@dgarske dgarske assigned jpbland1 and unassigned jpbland1 and wolfSSL-Bot Jan 17, 2024
where heap doesn't require a new ex function or struct field to avoid size increase
@jpbland1
Copy link
Copy Markdown
Contributor Author

Please add a test case for wolfSSL_X509_d2i_ex or adapt an existing one. How have you confirmed all X509 nodes have the heap hint? Did you try with WOLFSSL_HEAP_TEST?

Updated the test cases but the question of how to make sure all X509 nodes have the heap hint is why I didn't want to do a half measure on this but we can't blow up the x509small size. I've updated the functions that already have the heap hint and wouldn't require their own ex function and I guess if a customer needs those functions too we can add them but I think we need a long term solution so x509small doesn't break.

Tested working with WOLFSSL_HEAP_TEST.

Comparing the sizes, configured with ./configure --enable-opensslextra=x509small
On 089468f:

$ size -G /usr/local/lib/libwolfssl.so.42.0.0 
      text       data        bss      total filename
    445915     198363      24200     668478 /usr/local/lib/libwolfssl.so.42.0.0

On current commit:

$ size -G /usr/local/lib/libwolfssl.so.42.0.0 
      text       data        bss      total filename
    445979     198535      24200     668714 /usr/local/lib/libwolfssl.so.42.0.0

A difference of 236 bytes

dgarske
dgarske previously approved these changes Jan 18, 2024
@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske and jpbland1 Jan 18, 2024
Comment thread src/ssl_certman.c Outdated
Comment thread src/x509_str.c Outdated
@dgarske dgarske merged commit a3a7012 into wolfSSL:master Jan 19, 2024
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.

5 participants