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

Avoid questionable use of the value of a pointer that refers to deallocated space #2897

Closed
wants to merge 1 commit into from
Closed

Avoid questionable use of the value of a pointer that refers to deallocated space #2897

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion ssl/t1_lib.c
View file
Open in desktop
Expand Up @@ -3170,10 +3170,11 @@ static int tls_decrypt_ticket(SSL *s, const unsigned char *etick,
p = sdec;

sess = d2i_SSL_SESSION(NULL, &p, slen);
slen -= p - sdec;
Copy link
Contributor

Choose a reason for hiding this comment

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

need an int cast around the subtraction.

Copy link
Member

Choose a reason for hiding this comment

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

In some cases, that will still raise warnings. We have a PTRDIFF_T in crypto/evp/evp_enc.c that we probably should make wider use of for these kinds of things.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it has to be an int because slen is an int.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently there are no gcc and no clang warnings with --strict-warnings about this.
Only visual studio builds have one warning here.
However there are already many similar warnings all over in this function.

Sigh, it feels somehow wrong to defeat a warning here with a type-cast.
A type cast is just not checked at all, and even
slen -= (int)p; would no longer give me any warning.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind my previous (now removed) comment... d2i_SSL_SESSION() will of course change p...

Copy link
Member

Choose a reason for hiding this comment

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

The diff between two pointers will invariably cause a warning on VMS when building with 64-bit pointers, because the result is coerced to a 32-bit value, which may lose data. Not a risk in this case, but still an annoying warning. We've taken quite a bit of effort in other parts of our source to avoid this...

Copy link
Contributor

Choose a reason for hiding this comment

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

well then, what do folks want to do? @bernd-edlinger is right, it compiles without warning (my mistake). @levitte can VMS live with this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Worst case scenario, you'll see me adding pragmas in a day or two ;-)
Fine with me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I see, this makes it even worse...

Maybe it would be better to have a function that does the pointer-difference thing
in a clean way, and returns the remaining bytes in the input stream, so I could
use that directly as argument of the next d2i-function.

To resolve this issue, I will add a comment, what this statement is meant to do,
so it will hopefully be more clear, and add a type-cast to int.

The reason why I hate type-casts is: once the interfaces are cleaned up
the type-casts usually remain forever :-(

Copy link
Member

Choose a reason for hiding this comment

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

It turns out that VMS has no issues here... to my surprise, but I'm not complaining!

OPENSSL_free(sdec);
if (sess) {
/* Some additional consistency checks */
if (p != sdec + slen || sess->session_id_length != 0) {
if (slen != 0 || sess->session_id_length != 0) {
SSL_SESSION_free(sess);
return 2;
}
Expand Down