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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59a0962
crypto: move field initialization to class
Oct 12, 2018
d2e44f5
crypto: complete ClientHelloParser initializers
theholla Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions src/node_crypto_clienthello-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,7 @@ inline ClientHelloParser::ClientHelloParser()
: state_(kEnded),
onhello_cb_(nullptr),
onend_cb_(nullptr),
cb_arg_(nullptr),
session_size_(0),
session_id_(nullptr),
servername_size_(0),
servername_(nullptr),
ocsp_request_(0),
tls_ticket_size_(0),
tls_ticket_(nullptr) {
cb_arg_(nullptr) {
Reset();
}

Expand Down
20 changes: 10 additions & 10 deletions src/node_crypto_clienthello.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ class ClientHelloParser {
inline int ocsp_request() const { return ocsp_request_; }

private:
uint8_t session_size_;
const uint8_t* session_id_;
uint8_t session_size_ = 0;
const uint8_t* session_id_ = nullptr;
bool has_ticket_;
uint8_t servername_size_;
const uint8_t* servername_;
int ocsp_request_;
uint8_t servername_size_ = 0;
const uint8_t* servername_ = nullptr;
int ocsp_request_ = 0;

friend class ClientHelloParser;
};
Expand Down Expand Up @@ -108,16 +108,16 @@ class ClientHelloParser {
OnHelloCb onhello_cb_;
OnEndCb onend_cb_;
void* cb_arg_;
size_t frame_len_;
size_t body_offset_;
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?

uint16_t tls_ticket_size_;
const uint8_t* tls_ticket_;
uint16_t tls_ticket_size_ = -1;
const uint8_t* tls_ticket_ = nullptr;
};

} // namespace crypto
Expand Down