Skip to content

TS-4978: illegal memory access with ticket_key.filename#1120

Merged
SolidWallOfCode merged 1 commit intoapache:masterfrom
persiaAziz-zz:TS-4978
Nov 1, 2016
Merged

TS-4978: illegal memory access with ticket_key.filename#1120
SolidWallOfCode merged 1 commit intoapache:masterfrom
persiaAziz-zz:TS-4978

Conversation

@persiaAziz-zz
Copy link
Copy Markdown
Contributor

Avoiding illegal memory access by checking the return value of readConfigStringAlloc .

Comment thread iocore/net/SSLConfig.cc
ats_free(CACertRelativePath);

#if HAVE_OPENSSL_SESSION_TICKETS
REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just embed the call in the if?

if (REC_ERR_OKAY == REC_ReadConfigString(...)) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

was avoiding long statement

Comment thread iocore/net/SSLConfig.cc Outdated
ats_free_null(dhparamsFile);
ats_free_null(ssl_wire_trace_ip);
ats_free_null(ticket_key_filename);
ticket_key_filename = (char *)ats_free_null(ticket_key_filename);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's go bigger. Pull the code out of the constructor, make it a method, and then call that at the end of this method. Reset everything, really clean up.

@persiaAziz-zz
Copy link
Copy Markdown
Contributor Author

persiaAziz-zz commented Oct 18, 2016

Please review @SolidWallOfCode
Reset method added

@SolidWallOfCode
Copy link
Copy Markdown
Member

Looks good.

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Oct 27, 2016

Looks like we have merge conflicts now.

@persiaAziz-zz
Copy link
Copy Markdown
Contributor Author

persiaAziz-zz commented Oct 31, 2016

I will do a local merge and resolve

@persiaAziz-zz
Copy link
Copy Markdown
Contributor Author

The conflict is resolved. Please check

@SolidWallOfCode SolidWallOfCode merged commit fc16e79 into apache:master Nov 1, 2016
bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Jan 23, 2026
…he#1120)

* Add AuTest to reproduce apache#9275

This reproduces a variant of apache#9275 where we fail on the cache lookup. If
the cache lookup were to succeed, it should also fail on the cache write,
so this one test should cover both bugs.

* Reuse prepared cache write on redirected request

Fixes apache#9275.

When there is no parent proxy and ATS is following a redirect, it will do
a cache lookup on the redirected request. If that lookup results in a cache
miss, it will start to prepare a new cache write, which is wrong. The state
machine intentionally leaves the write open so we can re-use the connection;
this is possible to ascertain from the code and from comments to that effect.

We should only open a new write if we don't already have one when following
a redirect.

(cherry picked from commit 875463c)

Co-authored-by: JosiahWI <41302989+JosiahWI@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants