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.

Sign up for GitHub

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

Jump to bottom

crypto: move field initialization to class #23610

Closed
wants to merge 2 commits into from
Closed

crypto: move field initialization to class #23610

wants to merge 2 commits into from

Conversation

theholla
Copy link

@theholla theholla commented Oct 12, 2018 •
edited
Loading

Initialize fields of ClientHelloParser in class rather than in constructor.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 12, 2018
@jasnell jasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
addaleax
addaleax reviewed Oct 12, 2018
View reviewed changes
src/node_crypto_clienthello.h Outdated
size_t extension_offset_;
size_t frame_len_ = 0;
size_t body_offset_ = 0;
size_t extension_offset_ = 0;
uint8_t session_size_;
const uint8_t* session_id_;
uint16_t servername_size_;
const uint8_t* servername_;
uint8_t ocsp_request_;
Copy link
Member

Choose a reason for hiding this comment

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

I think you also need/want to add default initializers here, for the 5 that are later applied to ClientHello directly?

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the PR and moved the default initializers from ClientHello to ClientHelloParser, which I believe is what was needed in the first place.

Thanks for catching that. I'm not very familiar with C++ and appreciate the keen eye!

Copy link
Member

Choose a reason for hiding this comment

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

@theholla Just for context – I don’t think there was anything wrong with adding the initializers to ClientHello, as long as ClientHelloParser has all initializers that were previously in its own constructor :)

Copy link
Author

Choose a reason for hiding this comment

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

That's good to know, thanks! While you're here, I wonder if you see any issues with the two fields that are listed as one type in one class, and another in the next? Both servername_size and ocsp_request are listed as different types between ClientHello and ClientHelloParser.

class ClientHelloParser {
  class ClientHello {
    private:
      uint8_t servername_size_;
      int ocsp_request_;
  };
  private:
    uint16_t servername_size_ = 0;
    uint8_t ocsp_request_ = 0;
}

This predated the PR but stood out to me as a possible issue.

Copy link
Member

Choose a reason for hiding this comment

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

Puh … that’s a good question.

For ocsp_request_, I searched for instances of that in the source code, and it seems like the only values are 0 and 1 – I think you could change the type in one so it matches the other, if you like. (I could totally be wrong about this being a “boolean“, though…)

For servername_size_, I’m not sure. It looks to me like we wouldn’t do anything wrong by making both fields uint16_ts, and remove the static_cast when converting …

Either way, this might be better answered by @nodejs/crypto experts?

addaleax
addaleax approved these changes Oct 12, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks!

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/17779/ [scheduled]

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2018
ChALkeR
ChALkeR approved these changes Oct 12, 2018
View reviewed changes
Copy link
Member

@ChALkeR ChALkeR left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

Should be squashed (could be done at landing, I presume).

As a side note, it's interesting that tls_ticket_size_ = 0 appears unused, as it's immediately set to -1 in Reset() which is called directly from the constructor. But I would suggest not to touch that in this PR for now, this PR looks good.

@Trott
Copy link
Member

Trott commented Oct 13, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/1​77​98

@Trott
Copy link
Member

Trott commented Oct 13, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/17820/

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 13, 2018
@Trott
Copy link
Member

Trott commented Oct 13, 2018

Collaborators, please 👍 here to approve fast-tracking.

@refack refack removed the fast-track PRs that do not need to wait for 48 hours to land. label Oct 13, 2018
@addaleax
Copy link
Member

Landed in 98afca9

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@addaleax addaleax closed this Oct 15, 2018
addaleax pushed a commit that referenced this pull request Oct 15, 2018
@addaleax
crypto: move field initialization to class
98afca9 
PR-URL: #23610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@jasnell
crypto: move field initialization to class
ed0070e 
PR-URL: #23610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@addaleax
crypto: move field initialization to class
932cb33 
PR-URL: #23610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@MylesBorins
crypto: move field initialization to class
86a5690 
PR-URL: #23610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@rvagg
crypto: move field initialization to class
2c31ea7 
PR-URL: #23610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@MylesBorins
crypto: move field initialization to class
0e0c7b8 
PR-URL: #23610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.