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.

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

Modify vector's size instead of capacity to avoid bounds-checking failures #1630

Merged
merged 3 commits into from
Mar 5, 2022

Conversation

simpsonst
Copy link
Contributor

There are three commits, each to a distinct file. All involve using vector::resize instead of reserve, and vector::size instead of capacity, and the unmodified code produces a bounds-checking assertion failure in vector::operator[] when compiled with -D_GLIBCXX_ASSERTIONS. A smoke test was applied to the first two commits to verify that they eliminate a problem.
The first (in XrdMacaroonsHandler.cc) specifically fixes #1614. Although already closed, I think this change would be more readable.
The second (in XrdTpcStream.hh) is in code that was not engaged/discovered during the smoke test until the first was fixed.
The third (in XrdThrottleManager.cc) was found just by a scan of the code, and is not engaged by our configuration. However, adding xrootd.fslib throttle default does engage it, and causes the assertion failure as soon as the process starts up without the fix.

Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

It would appear that this pull request is based on a previous version. Specifically, line 572 was added to XrdMacaroonsHandler.cc and should have appeared as

macaroon_resp.push_back((char)0);

and does not appear in this pull request. Could you rebase your branch to the latest master and resubmit the pull request?

@simonmichal
Copy link
Contributor

@simpsonst : thanks a lot for your PR! My understanding is that those 3 commits are yours:

Set vector's logical size to avoid bounds-checking failure.
Vector must be resized to prevent bounds-checking assertion failure.
Set vector's logical size to avoid bounds-checking failure.

Now, we need to rework the PR so it contains only those 3 commits, unfortunately in the current form we cannot merge it because it pollutes the history, which in turn makes it very difficult to cut bugfix releases. What I would suggest is that you either:

  • remove the redundant commits from your branch and then rebase on master, or
  • create a new branch from master, cherry-pick the 3 commits and then replace the PR branch

Let me know if you need any help!

@simpsonst
Copy link
Contributor Author

Thanks @simonmichal, I figured I might have butchered it.
(The third commit is bac553b; the third you listed I think is the same as the first.)
Going for the second option, I've done:

git checkout master
git checkout -b fix-vector-reserve
git cherry-pick 989e088ca3375d95b035abd0130e78689d4d28c9 bac553b75df1427e119699345c9a766f5792ddb3 666d8acaf162a49a1851cab220a5c5b4740dc776

That's produced a conflict on src/XrdMacaroons/XrdMacaroonsHandler.cc, so I've resolved that, and continued with:

git cherry-pick --continue

...and used the default commit message. I presume the next step is to push my local fix-vector-reserve over the fix-macaroons-reserve on the fork. From the new branch, I guess it's:

git push --force origin fix-macaroons-reserve

But please advise!

@simonmichal
Copy link
Contributor

@simpsonst : I think you first need to replace the local version of fix-macaroons-reserve:

  • git checkout fix-macaroons-reserve
  • git reset --hard fix-vector-reserve
  • git push -f

@simpsonst
Copy link
Contributor Author

Done(?)! Thanks.

@simonmichal
Copy link
Contributor

Perfect, thanks a lot! I think it can be merged now :-)

@abh3 abh3 merged commit bfc0ca2 into xrootd:master Mar 5, 2022
@simpsonst simpsonst deleted the fix-macaroons-reserve branch November 17, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XrdMacaroon segfaults on client auth (RHEL8 host)
3 participants