Replace 'ssl3_get_message()' with 'tls_get_message_body()'

[heygauri]

Update commit messages that previously used ssl3_get_message() to now use tls_get_message_body() due to the split into tls_get_message_header and tls_get_message_body in OpenSSL 1.1.0.

Fixes #21582

[t8m]

Do not edit the CHANGES.md as that should reflect the history as of when the concrete change was done and not the current state of the code.

[heygauri]

Okay

[t8m]

You still did not revert this change.

[heygauri]

I am sorry I forgot this one.

[mattcaswell]

Note that there are more references that need updating. One in ssl/record/rec_layer_d1.c and one in ssl/record/rec_layer_s3.c

[mattcaswell]

In this case its tls_get_message_header or tls_get_message_body (they both make such a call):

openssl/ssl/statem/statem_lib.c

Lines 1530 to 1546 in 0f9caad

	int tls_get_message_header(SSL_CONNECTION *s, int *mt)
	{
	/* s->init_num < SSL3_HM_HEADER_LENGTH */
	int skip_message, i;
	uint8_t recvd_type;
	unsigned char *p;
	size_t l, readbytes;
	SSL *ssl = SSL_CONNECTION_GET_SSL(s);
	p = (unsigned char *)s->init_buf->data;
	do {
	while (s->init_num < SSL3_HM_HEADER_LENGTH) {
	i = ssl->method->ssl_read_bytes(ssl, SSL3_RT_HANDSHAKE, &recvd_type,
	&p[s->init_num],
	SSL3_HM_HEADER_LENGTH - s->init_num,
	0, &readbytes);

and

openssl/ssl/statem/statem_lib.c

Lines 1640 to 1657 in 0f9caad

	int tls_get_message_body(SSL_CONNECTION *s, size_t *len)
	{
	size_t n, readbytes;
	unsigned char *p;
	int i;
	SSL *ssl = SSL_CONNECTION_GET_SSL(s);
	if (s->s3.tmp.message_type == SSL3_MT_CHANGE_CIPHER_SPEC) {
	/* We've already read everything in */
	*len = (unsigned long)s->init_num;
	return 1;
	}
	p = s->init_msg;
	n = s->s3.tmp.message_size - s->init_num;
	while (n > 0) {
	i = ssl->method->ssl_read_bytes(ssl, SSL3_RT_HANDSHAKE, NULL,
	&p[s->init_num], n, 0, &readbytes);

[heygauri]

Yeah even I was confused at this point. But I thought that since tls_get_message_header is calling the ssl_read_bytes. I can go for it. But honestly I do not see either of them calling "ssl3" to be specific. It might be deep down in the nested calls. I tried looking for it but didnt find much.

sumitra@sumitra:~/opensource/openssl$ git grep 'ssl3_read_bytes' -- .
CHANGES.md: ssl3_read_bytes() found application data while handshake
CHANGES.md: * Fix ssl3_read_bytes (ssl/s3_pkt.c): To ignore messages of unknown
CHANGES.md: A 'peek' parameter has also been added to ssl3_read_bytes, which
doc/man3/ERR_put_error.pod:descriptions. For example, the function ssl3_read_bytes() reports a
ssl/record/rec_layer_s3.c:int ssl3_read_bytes(SSL *ssl, uint8_t type, uint8_t *recvd_type,
ssl/record/record.h:__owur int ssl3_read_bytes(SSL *s, uint8_t type, uint8_t *recvd_type,
ssl/s3_lib.c: * ssl3_read_bytes decided to call s->handshake_func, which called
ssl/s3_lib.c: * ssl3_read_bytes to read handshake data. However, ssl3_read_bytes
ssl/ssl_local.h: ssl3_read_bytes,
ssl/ssl_local.h: ssl3_read_bytes, \

You can see there is no direct call to ssl3_read_bytes anywhere.

PS: I am new to open source. I apologise for the mistakes made.

[mattcaswell]

You can see there is no direct call to ssl3_read_bytes anywhere.

Ah, yes. That is because the call is made via a function pointer. So, in tls_get_message_header you will see a call to ssl->method->ssl_read_bytes which (in this case) is a function pointer to ssl3_read_bytes.

I apologise for the mistakes made.

There is no need to apologise. We don't expect new contributors to know the details of the source code and I hope our review comments help you learn your way around.

[heygauri]

Okay. I got it thanks.

I will then mention both the functions tls_get_message_header and tls_get_message_body in the comments.

[mattcaswell]

In this case I would just delete the text in parens after SSL3_RT_HANDSHAKE. It actually makes no sense any more. This function is called from quite a number of places with the type set to SSL3_RT_HANDSHAKE, and tls_get_message_header isn't one of them (dtsl1_read_bytes is only relevant to DTLS, and tls_get_message_header is only relevant to TLS).

[heygauri]

Okay.

[mattcaswell]

You changed back the wrong entry in CHANGES.md to ssl3_get_message....this entry was correctly referring to tls_get_message_header(). You should have changed back the entry on line 15565 :-)

[heygauri]

Oh I see. Thanks again!

[t8m]

please split the line so it is not longer than 80 characters

[heygauri]

Okay.

[t8m]

Ahem, this should be kept as is.

Please squash the commits with git rebase -i master and drop all the changes in CHANGES.md. There should not be any.

[heygauri]

Okay.

[heygauri]

Hi @t8m I've encountered significant issues in the ssl3_to_tls_message_comments branch, leading to a complex situation. Considering this, I'm contemplating creating a new branch and reapplying the changes to ensure a cleaner and more manageable state. Will that be fine?

[tom-cosgrove-arm]

Adding a second function makes the subject of the verb "call" plural, so this should be "call us" not "calls us"

[heygauri]

Okay.

[tom-cosgrove-arm]

@heygauri You need to push your changes to your OpenSSL fork, probably with -f if you've rebased

[heygauri]

Hello @tom-cosgrove-arm, I'm considering the idea of creating a new branch and reapplying the changes. Unfortunately, I made quite a mess in the ssl3_to_tls_message_comments branch due to my limited knowledge of the process.

[tom-cosgrove-arm]

Hello @tom-cosgrove-arm, I'm considering the idea of creating a new branch and reapplying the changes. Unfortunately, I made quite a mess in the ssl3_to_tls_message_comments branch due to my limited knowledge of the process.

Yes, that's fine. You can close this PR and create a new one

[tom-cosgrove-arm]

(do note though, that you could delete your local branch with the problems - or rename it to something like ssl3_to_tls_message_comments-bad - then create a new branch with the original name, and git push -f that new branch to your fork)

[heygauri]

Hello @tom-cosgrove-arm, I've followed your instructions to delete the local branch and start fresh. I'd like to highlight that I now need to drop or squash some commits based on the feedback from the maintainers. However, I'm encountering an issue: when I perform the rebase using the command "git rebase -i master," my branch seems to change automatically (as shown in the reference below). How can I ensure that the rebase changes are made in the branch "ssl3_to_tls_message_comments"? Your guidance would be appreciated.

sumitra@sumitra:~/opensource/openssl$ git branch

• (no branch, rebasing ssl3_to_tls_message_comments)
  master
  ssl3_to_tls_message_comments

[tom-cosgrove-arm]

Okay, this is what I would do in your situation, including renaming the old branch. There are several ways to do this, some more "git pro user" than this!

I don't know if you need to make more changes, but this should give you something to start afresh from.

First, get onto the master branch

$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

Now rename the branch

$ git branch -m ssl3_to_tls_message_comments ssl3_to_tls_message_comments-BAD

Create a new ssl3_to_tls_message_comments branch

$ git checkout -b ssl3_to_tls_message_comments
Switched to a new branch 'ssl3_to_tls_message_comments'

Now, there are four commits in your old branch that I see, with most recent first:

$ git log --oneline -n4 ssl3_to_tls_message_comments-BAD
fa1a5ac595 (ssl3_to_tls_message_comments-BAD) Update accurate function names in CHANGES.md
13b62e2002 Replace ssl3_get_message() with tls_get_message_header() and/or tls_get_message_body()
801621c391 Replace 'ssl3_get_message()' with 'tls_get_message_header()'
f7b2bfbc4f Replace 'ssl3_get_message()' with 'tls_get_message_body()'

Depending what you've done, the commit IDs you have might be different.

We don't want the most recent commit, as it only touches CHANGES.md, but we'll bring the other commits, from oldest to newest (i.e. in the opposite order to that from git log), into our clean branch with cherry-pick. Remember, we changed back to master, then created a new branch, so the code is just what was in master.

$ git cherry-pick f7b2bfbc4f
Auto-merging ssl/ssl_local.h
Auto-merging CHANGES.md
[ssl3_to_tls_message_comments c22ecd3a9e] Replace 'ssl3_get_message()' with 'tls_get_message_body()'
 Author: Sumitra Sharma <sumitraartsy@gmail.com>
 Date: Tue Aug 29 16:02:47 2023 +0530
 2 files changed, 2 insertions(+), 2 deletions(-)

$ git cherry-pick 801621c391
Auto-merging ssl/ssl_local.h
[ssl3_to_tls_message_comments 77115561ba] Replace 'ssl3_get_message()' with 'tls_get_message_header()'
 Author: Sumitra Sharma <sumitraartsy@gmail.com>
 Date: Tue Aug 29 18:16:11 2023 +0530
 3 files changed, 3 insertions(+), 3 deletions(-)

$ git cherry-pick 13b62e2002
Auto-merging CHANGES.md
[ssl3_to_tls_message_comments b1d6539bd0] Replace ssl3_get_message() with tls_get_message_header() and/or tls_get_message_body()
 Author: Sumitra Sharma <sumitraartsy@gmail.com>
 Date: Tue Aug 29 20:37:21 2023 +0530
 3 files changed, 3 insertions(+), 3 deletions(-)

Now, we need the commit message. I'm going to snag it from the most recent commit and put it into a file called /tmp/message, but you might want to rewrite it

$ git show | head -15 | tail -11 | sed 's/^  *//' | tee /tmp/message
Replace ssl3_get_message() with tls_get_message_header() and/or tls_get_message_body()

Update commit messages that previously used ssl3_get_message()
to now use tls_get_message_header() and tls_get_message_body()
due to the split in OpenSSL 1.1.0.

CLA: trivial

Fixes #21582

Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>

Now, use git reset to wind back this branch pointer, but keep the changes that we cherry-picked:

$ git reset HEAD~3
Unstaged changes after reset:
M	CHANGES.md
M	ssl/record/rec_layer_d1.c
M	ssl/record/rec_layer_s3.c
M	ssl/ssl_local.h

We don't want to modify CHANGES.md, so restore it

$ git restore CHANGES.md

$ git status
On branch ssl3_to_tls_message_comments
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
  (commit or discard the untracked or modified content in submodules)
	modified:   ssl/record/rec_layer_d1.c
	modified:   ssl/record/rec_layer_s3.c
	modified:   ssl/ssl_local.h

Now we can commit again

$ git commit -F/tmp/message -a
[ssl3_to_tls_message_comments e4a6f2fe12] Replace ssl3_get_message() with tls_get_message_header() and/or tls_get_message_body()
 3 files changed, 3 insertions(+), 3 deletions(-)

and you can use git show to check that you are happy with what you've got, then

$ git push -f origin ssl3_to_tls_message_comments

will replace the ssl3_to_tls_message_comments branch in your fork with what you've done

[tom-cosgrove-arm]

(to get out of "(no branch, rebasing ssl3_to_tls_message_comments)" just use git rebase --abort)

[heygauri]

@tom-cosgrove-arm Thank you for the thorough explanation. I've submitted a pull request (PR). Please review it and let me know if there are any concerns or further adjustments needed. Your feedback is highly appreciated.

[tom-cosgrove-arm]

@heygauri It looks like you have merged master on top of the changes you previously had - if you look at the Commit tab you see your four commits from 29 August, then lots from master, then another commit from you. This should just show the commits from you, which is why I suggested re-creating the branch from master and starting afresh with git cherry-pick to take the changes that you want

[heygauri]

Okay. I will redo this.

[tom-cosgrove-arm]

LGTM

[openssl-machine]

This pull request is ready to merge

[hlandau]

Merged to master. Thank you.