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

include/denc: remove nullptr runtime magic boundedness check #13889

Merged
merged 1 commit into from Mar 24, 2017

Conversation

Projects
None yet
4 participants
@liewegas
Member

liewegas commented Mar 8, 2017

We were passing (const T)nullptr to bound_encode so that we
would segv at runtime in a bound_encode implementation had anyd
dependency on the value. However, this relies on undefined
language behavior and fails on low optimization levels.

So drop these checks and rely on the developer to implement a
legal bound_encode()

Signed-off-by: Sage Weil sage@redhat.com

@liewegas liewegas requested a review from adamemerson Mar 8, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Mar 9, 2017

@liewegas
I dropped the github-diff on my work tree
Clang is not quite happy ..... so something might be missing?

In file included from /usr/srcs/Ceph/work/ceph/src/common/SloppyCRCMap.cc:4:
In file included from /usr/srcs/Ceph/work/ceph/src/common/SloppyCRCMap.h:7:
In file included from /usr/srcs/Ceph/work/ceph/src/include/types.h:21:
In file included from /usr/srcs/Ceph/work/ceph/src/include/uuid.h:8:
In file included from /usr/srcs/Ceph/work/ceph/src/include/encoding.h:25:
/usr/srcs/Ceph/work/ceph/src/include/denc.h:684:2: error: no matching function for call to 'denc'
        denc(*s.begin(), elem_size);
        ^~~~
/usr/srcs/Ceph/work/ceph/src/include/denc.h:1313:11: note: in instantiation of function template specialization
      '_denc::container_base<std::map, _denc::maplike_details<std::__1::map<unsigned long, unsigned int,
      std::__1::less<unsigned long>, std::__1::allocator<std::__1::pair<const unsigned long, unsigned int> > > >, unsigned
      long, unsigned int, std::__1::less<unsigned long>, std::__1::allocator<std::__1::pair<const unsigned long, unsigned
      int> > >::bound_encode<std::__1::pair<unsigned long, unsigned int> >' requested here
  traits::bound_encode(o, len);
          ^
@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Mar 9, 2017

I'm fixing it up as we speak, I should have a diff to throw up in a bit.

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 9, 2017

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Mar 9, 2017

Here is a fix!

diff --git a/src/include/denc.h b/src/include/denc.h
index 243f096..112450b 100644
--- a/src/include/denc.h
+++ b/src/include/denc.h
@@ -681,7 +681,7 @@ namespace _denc {
       size_t elem_size = 0;
       p += sizeof(uint32_t);
       if (!s.empty()) {
-       denc(*s.begin(), elem_size);
+       denc(static_cast<const T&>(*s.begin()), elem_size);
        p += sizeof(uint32_t) + elem_size * s.size();
       }
     }
@@ -703,7 +703,7 @@ namespace _denc {
       size_t elem_size = 0;
       p += sizeof(uint32_t);
       if (!s.empty()) {
-       denc(*s.begin(), elem_size, f);
+       denc(static_cast<const T&>(*s.begin()), elem_size, f);
        p += elem_size * s.size();
       }
     }

You may ask "@adamemerson! What the heck is the point of that cast?"

If you do ask this, I shall tell you that maps, sets, multimaps, multisets, unordered_maps, unordered_sets, and associative containers generally don't have the value type you would expect. Iterators into maps do not return std::pair<key, value>& but instead std::pair<const key, value>&. This is why we define T explicitly in the container traits. For most containers it's just value_type, but for map it's explicitly std::pair<key, value>.

Without this the calls to denc fail, because there is no denc_traits for, for example, const int. Sticking the cast in just ensures we call denc on a reference of the appropriate type.

include/denc: remove nullptr runtime magic boundedness check
We were passing *(const T*)nullptr to bound_encode so that we
would segv at runtime in a bound_encode implementation had anyd
dependency on the value.  However, this relies on undefined
language behavior and fails on low optimization levels.

So drop these checks and rely on the developer to implement a
legal bound_encode()

Signed-off-by: Sage Weil <sage@redhat.com>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Mar 10, 2017

LGTM
FreeBSD/Clang compiles all the way to the end.
Have not yet run the tests, but there I did not have any trouble in the past.

@liewegas liewegas added the needs-qa label Mar 10, 2017

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Mar 10, 2017

Lithospheres Grant Tenancy to Mamga

tchaikov added a commit to tchaikov/ceph that referenced this pull request Mar 18, 2017

os/bluestore: do not use nullptr to calc the size of bluestore_pextent_t
see also ceph#13889, otherwise this segfaults when compiled with -O0.

Signed-off-by: Kefu Chai <kchai@redhat.com>

@liewegas liewegas merged commit 2d32138 into ceph:master Mar 24, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the liewegas:wip-denc-nullptr branch Mar 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment