Skip to content

Comments

Restrict usage of bio_dgram_sctp_data only to DGRAM SCTP methods#9216

Closed
raja-ashok wants to merge 1 commit intoopenssl:masterfrom
raja-ashok:master-dgram-sctp
Closed

Restrict usage of bio_dgram_sctp_data only to DGRAM SCTP methods#9216
raja-ashok wants to merge 1 commit intoopenssl:masterfrom
raja-ashok:master-dgram-sctp

Conversation

@raja-ashok
Copy link
Contributor

@raja-ashok raja-ashok commented Jun 21, 2019

The ptr member of BIO should be dereferenced only on functions of BIO_METHOD. But in DGRAM SCTP it is accessed outside. That means members of bio_dgram_sctp_data are accessed other than BIO_METHOD functions like BIO_dgram_sctp_notification_cb, BIO_dgram_sctp_wait_for_dry and BIO_dgram_sctp_msg_waiting. I feel this should be avoided.

So added new control command to set the members (handle_notifications and notification_context) required to access outside BIO_METHOD functions. And also made BIO_dgram_sctp_wait_for_dry() and BIO_dgram_sctp_msg_waiting() as part of control function.

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

@raja-ashok raja-ashok force-pushed the master-dgram-sctp branch 2 times, most recently from 28b337b to b3eb552 Compare July 7, 2019 03:22
@raja-ashok
Copy link
Contributor Author

Requesting to review and provide comments on this change.

@raja-ashok
Copy link
Contributor Author

Now the changes are to make BIO_dgram_sctp_msg_waiting() and BIO_dgram_sctp_wait_for_dry() to call BIO_ctrl to perform the operation. And we can keep BIO_dgram_sctp_notification_cb() as it is. In case of custom SCTP BIO usage, application should avoid calling this API.

@raja-ashok
Copy link
Contributor Author

Or shall we call BIO_callback_ctrl() in BIO_dgram_sctp_notification_cb() to set function pointers. But this requires some type casting fp while calling BIO_callback_ctrl(). Please provide your opinion.

@mattcaswell
Copy link
Member

we can keep BIO_dgram_sctp_notification_cb() as it is.

I think we just leave it as it is for now.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Aug 5, 2019
@mattcaswell
Copy link
Member

Needs second review.

@t8m t8m added the branch: master Applies to master branch label Aug 8, 2019
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

LGTM

@mattcaswell mattcaswell 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 Aug 9, 2019
@mattcaswell
Copy link
Member

Pushed. Thanks.

@mattcaswell mattcaswell closed this Aug 9, 2019
levitte pushed a commit that referenced this pull request Aug 9, 2019
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #9216)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants