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

Do not include sparse_array.o in libssl #22111

Closed
wants to merge 1 commit into from

Conversation

huiyuexu
Copy link
Contributor

@huiyuexu huiyuexu commented Sep 15, 2023

sparse_array.o is not needed in libssl at 3.0.x version.

fix #22113

Checklist
  • documentation is added or updated
  • tests are added or updated

sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>
@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Sep 15, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

CI fuzz failure is unrelated, and I've raised issue #22114 to cover the broken 3.0 branch

@tom-cosgrove-arm tom-cosgrove-arm added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 15, 2023
@huiyuexu
Copy link
Contributor Author

How I can rebuild the CIFuzz? @tom-cosgrove-arm

@t8m t8m added tests: exempted The PR is exempt from requirements for testing branch: 3.1 Merge to openssl-3.1 and removed branch: master Merge to master branch branch: 3.1 Merge to openssl-3.1 labels Sep 15, 2023
@t8m
Copy link
Member

t8m commented Sep 15, 2023

This is for 3.1 and 3.0 branches only as sparse arrays are used by QUIC.

@t8m
Copy link
Member

t8m commented Sep 15, 2023

It is possible to do something for the master branch as well - i.e., make the sparse array added only when QUIC is enabled. However that would be a different patch.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale
Copy link
Contributor

Do we even need this for QUIC?

@levitte
Copy link
Member

levitte commented Sep 18, 2023

Why would we need this for QUIC? I just grepped for ossl_sa_ and OPENSSL_SA through the source, and I can't see it used anywhere except for the property code (in libcrypto) and a test program.

@levitte
Copy link
Member

levitte commented Sep 18, 2023

Ah, found the spot where it might look like libssl uses sparse arrays. However, it builds fine after this diff too:

diff --git a/ssl/event_queue.c b/ssl/event_queue.c
index 49890a36b5..20f4020db5 100644
--- a/ssl/event_queue.c
+++ b/ssl/event_queue.c
@@ -9,7 +9,6 @@
 
 #include <stdlib.h>
 #include "internal/event_queue.h"
-#include "crypto/sparse_array.h"
 #include "ssl_local.h"
 
 struct ossl_event_queue_st {

@paulidale
Copy link
Contributor

I saw that, it's just the header file and isn't actually used.

@huiyuexu
Copy link
Contributor Author

Could this submission be merged?

@paulidale
Copy link
Contributor

Our automation didn't send a reminder here. It's meant to.

Possibly because this PR is against the 3.0 branch not master?

@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 22, 2023
@t8m
Copy link
Member

t8m commented Sep 22, 2023

So, should this be merged to master as well?

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.

So, should this be merged to master as well?

I think it should be. My approval counts for master/3.1 and 3.0. It sounds like there should be a follow on PR to remove the spurious header file inclusion.

@mattcaswell
Copy link
Member

It sounds like there should be a follow on PR to remove the spurious header file inclusion.

#22174

@t8m
Copy link
Member

t8m commented Sep 22, 2023

Merged to master, 3.1, and 3.0 branches. Thank you for your contribution.

@t8m t8m closed this Sep 22, 2023
openssl-machine pushed a commit that referenced this pull request Sep 22, 2023
sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22111)

(cherry picked from commit a31cd07)
openssl-machine pushed a commit that referenced this pull request Sep 22, 2023
sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22111)
openssl-machine pushed a commit that referenced this pull request Sep 22, 2023
sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22111)

(cherry picked from commit a31cd07)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22111)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22111)

(cherry picked from commit a31cd07af1ca34cdbbd2b077a933208d447ed0b2)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
sparse_array.o is not needed in libssl at 3.0.x version.

Signed-off-by: Huiyue Xu <xuhuiyue@huawei.com>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22111)

(cherry picked from commit a31cd07af1ca34cdbbd2b077a933208d447ed0b2)
Signed-off-by: fly2x <fly2x@hitls.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants