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

TS-4583: Null-pointer dereference after check #802

Closed
wants to merge 1 commit into from
Closed

TS-4583: Null-pointer dereference after check #802

wants to merge 1 commit into from

Conversation

alhonen
Copy link
Contributor

@alhonen alhonen commented Jul 17, 2016

Fixes Coverity issue CID 1021958: HttpSM.cc checks server_entry against
NULL, suggesting it might be NULL, before calling
release_server_session(), which dereferences server_entry.

@zwoop zwoop added the HTTP label Jul 17, 2016
@zwoop zwoop added this to the 7.0.0 milestone Jul 17, 2016
@zwoop zwoop self-assigned this Jul 17, 2016
@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

[approve ci]

@atsci
Copy link

atsci commented Jul 17, 2016

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/334/ for details.

@atsci
Copy link

atsci commented Jul 17, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/440/ for details.

@jpeach
Copy link
Contributor

jpeach commented Jul 18, 2016

I don't think this change is correct. It seems to me that the invariant is that server_session and server_entry should be NULL or non-NULL together. So if we call release_server_session() when server_entry is NULL, we ought to hit the NULL check on server_session as well.

@zwoop
Copy link
Contributor

zwoop commented Jul 23, 2016

@alhonen Thoughts on the concerns from James?

@alhonen
Copy link
Contributor Author

alhonen commented Jul 25, 2016

If they indeed do go NULL together (a bit hard to verify), then we can change the null check instead. I'll modify it thus.

TS-4583: Null-pointer dereference after check
88de790 
Fixes Coverity issue CID 1021958: HttpSM.cc checks server_entry against
NULL, suggesting it might be NULL, before calling
release_server_session(), which dereferences server_entry. As
server_session would be NULL along with server_entry, change to only
call release_server_session if server_entry is non-NULL.
@atsci
Copy link

atsci commented Jul 25, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/479/ for details.

@atsci
Copy link

atsci commented Jul 25, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/376/ for details.

@bryancall bryancall self-assigned this Sep 8, 2016
@zwoop zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016
@PSUdaemon PSUdaemon requested a review from zwoop December 16, 2016 17:06
@bryancall
Copy link
Contributor

bryancall commented Dec 22, 2016 •
edited

It is correct to check the value of the server_entry is not NULL before dereferencing the pointer in the same conditional. This is a common pattern.

The method release_server_session dereferences dereference server_entry, so it is most likely stopped by the servers_session NULL pointer check before the dereference. This is more of a side effect and shouldn't be relied on.

@bryancall
Copy link
Contributor

Didn't add "This closes #802 to 72e5181, so I am going to manually close.

@bryancall bryancall closed this Dec 22, 2016
@zwoop zwoop modified the milestone: 7.1.0 May 4, 2017
bneradt added a commit to bneradt/trafficserver that referenced this pull request Jun 6, 2023
@bneradt
Add back push_method_enabled enforcement (apache#802)
c027ab8 
proxy.config.http.push_method_enabled. This adds back the enforcemenent
of this configuration so that hosts can only PUSH when
proxy.config.http.push_method_enabled is 1.

(cherry picked from commit f725f3fd2e66da4a3e6236419bdd8e454c1a8917)
JosiahWI pushed a commit to JosiahWI/trafficserver that referenced this pull request Jul 19, 2023
@bneradt
Add back push_method_enabled enforcement (apache#802)
9f6e913 
proxy.config.http.push_method_enabled. This adds back the enforcemenent
of this configuration so that hosts can only PUSH when
proxy.config.http.push_method_enabled is 1.

(cherry picked from commit f725f3fd2e66da4a3e6236419bdd8e454c1a8917)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zwoop zwoop Awaiting requested review from zwoop

@bryancall bryancall Awaiting requested review from bryancall

Assignees

@bryancall bryancall

Labels
HTTP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alhonen @zwoop @atsci @jpeach @bryancall