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

dereferenceable::operator-> uses potentially overloaded T::operator& #35

Closed
joaquintides opened this issue Nov 23, 2017 · 6 comments
Closed
Assignees

Comments

@joaquintides
Copy link
Member

In line 305 of boost/operators.hpp:

return &*static_cast<const T&>(*this);

This will break if T overloads operator&. I suggest using boost::addressof to protect us from that.

@d-frey d-frey self-assigned this Nov 23, 2017
@d-frey
Copy link
Member

d-frey commented Nov 23, 2017

Looking at the documentation at http://www.boost.org/doc/libs/1_65_1/libs/utility/operators.htm#deref, it is documented to return &*i, which must be convertible to P. Given that the library is quite old and it has been like that for >=15 years, I wonder if there really is a problem that needs to be fixed. Plus we would add a dependency. What are your thoughts on this? Or do you have a specific problem that needs fixing?

@joaquintides
Copy link
Member Author

joaquintides commented Nov 23, 2017

Hi Daniel,

It's a real problem reported by a user of Boost.MultiIndex —which library is BTW 13 years old :-)

https://stackoverflow.com/questions/47416304/boostmulti-index-container-crashes-with-com-ptr-t-objects

Basically, Boost.MultiIndex does not work with elements whose operator& is overloaded. The problem is scattered through the code, one of the locations being its dependency on boost/operators.hpp.

I understand changing this on your side requires that you update the documentation from

Requirements: (&*i). Return convertible to P.

to

Requirements: *i. Return convertible to R.

which is not, strictly speaking, a relaxation of the former condition —*i could in principle be anything as long as &*i returns a pointer. So it is up to you; if you decide to not do the change I can fix the issue on my side, although it's more verbose in terms of LOCs touched and the root problem might hit other code using boost/operators.hpp. FWIW commercial implementations of std containers guard themselves against abuses of operator& overloading via constructs similar to boost::addressof.

Thank you

@d-frey
Copy link
Member

d-frey commented Nov 23, 2017

If Boost.MultiIndex is only 13 years old, you loose ;-P

Joking aside, I am not trying to ignore the problem, I was just curious where this was coming from. I think it is likely enough that people are not relying on the current implementation to allow them to "abuse" an overloaded operator&, so fixing it by using boost::addressof seems reasonable.

For the documentation part, well, it's not correct that the "return" is directly convertible to R. Maybe this would work:

Requirements: *i. Pointer to the return convertible to R.

or "...returned value..." instead of "...return..."?

@joaquintides
Copy link
Member Author

joaquintides commented Nov 23, 2017

Requirements: *i. Pointer to the return convertible to R.

I think this not correct either... What about

Requirements: *i. The address of the returned value convertible to P.

"Address of a value" is a well-defined concept that does not depend on operator&, so I think this holds water.

@d-frey
Copy link
Member

d-frey commented Nov 23, 2017

OK, agreed. I'll commit a fix soon.

@joaquintides
Copy link
Member Author

Thank you! Some Russian guy is going to be grateful for this, even if he is not aware of the behind-the-scenes conversation.

@joaquintides joaquintides changed the title dereferenceable::operator& uses potentially overloaded T::operator& dereferenceable::operator-> uses potentially overloaded T::operator& Nov 23, 2017
@d-frey d-frey closed this as completed in ad0fc7c Nov 23, 2017
d-frey added a commit that referenced this issue Dec 1, 2017
Protect dereferenceable<> against overloaded operator&, fixes #35
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

No branches or pull requests

2 participants