Skip to content

Update XPACK.cc to fix use-of-uninitialized-pointer-field case#11287

Merged
shukitchan merged 7 commits into
apache:masterfrom
shukitchan:shukitchan-patch-11
Apr 30, 2024
Merged

Update XPACK.cc to fix use-of-uninitialized-pointer-field case#11287
shukitchan merged 7 commits into
apache:masterfrom
shukitchan:shukitchan-patch-11

Conversation

@shukitchan
Copy link
Copy Markdown
Contributor

Fuzzing reveals a use-of-uninitialized-pointer-field case

https://oss-fuzz.com/testcase-detail/6285050312196096

I think this should fix the problem

@shukitchan shukitchan marked this pull request as draft April 24, 2024 22:21
@shukitchan shukitchan marked this pull request as ready for review April 24, 2024 22:56
@shukitchan shukitchan requested a review from maskit April 24, 2024 22:56
@shukitchan shukitchan self-assigned this Apr 24, 2024
@shukitchan shukitchan added this to the 10.1.0 milestone Apr 24, 2024
@maskit
Copy link
Copy Markdown
Member

maskit commented Apr 24, 2024

I'm not too sure if we should do this, the memset. Use of uninitialized field implies reading an entry that does not exist. I think we should check index numbers instead so such access does not happen. Touching the entire buffer would suppress the report, but it can hide real issues.

@shukitchan
Copy link
Copy Markdown
Contributor Author

Let me give it more thoughts

@shukitchan
Copy link
Copy Markdown
Contributor Author

@maskit
I double checked and i think it should be the fix we should do.

If we start from the beginning and insert an oversized entry, _make_space() will always look at the tail entry. So memset to 0 will make we fail gracefully. That's the problem the fuzz test is having.

If we already have entries and now add an oversized entry, we need _make_space() to at least clear out all entries first before giving up. So we can't have a precondition check for an oversized entry. That is implied in the unit test - https://github.com/apache/trafficserver/blob/master/src/proxy/hdrs/unit_tests/test_XPACK.cc#L318

@maskit
Copy link
Copy Markdown
Member

maskit commented Apr 29, 2024

Shouldn't we call is_empty() at the beginning of _make_space() and return false if the table is empty?

@shukitchan
Copy link
Copy Markdown
Contributor Author

Shouldn't we call is_empty() at the beginning of _make_space() and return false if the table is empty?

I agree. I just made a change for that.

@shukitchan
Copy link
Copy Markdown
Contributor Author

[approve ci fedora]

@shukitchan
Copy link
Copy Markdown
Contributor Author

[approve ci autest]

@shukitchan
Copy link
Copy Markdown
Contributor Author

@maskit it is ready for review again. Thanks

@shukitchan shukitchan merged commit 9ebe5a2 into apache:master Apr 30, 2024
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 May 1, 2024
@cmcfarlen
Copy link
Copy Markdown
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request May 1, 2024
* Update XPACK.cc

* Update XPACK.cc

* Update XPACK.cc

* Update XPACK.cc

* Update XPACK.cc

* Update XPACK.cc

* Update XPACK.cc

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

Labels

Projects

Status: picked-10.0.0

Development

Successfully merging this pull request may close these issues.

4 participants