Work around memory-aliasing in descriptor ParsePubkey #19508

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2007-noAliasMemDesc changing 2 files +5 −3

MarcoFalke commented at 8:00 PM on July 13, 2020: member

While this is not undefined behaviour, the memory aliasing trick is confusing when reading the code. Having a.size()==0 and then access a[0] works in this particular case, but should probably be avoided to harden the code for the future.
Work around memory-aliasing in descriptor ParsePubkey fa8a992589
MarcoFalke added the label Refactoring on Jul 13, 2020
practicalswift commented at 9:21 PM on July 13, 2020: contributor

Concept ACK: simpler is better
MarcoFalke commented at 7:45 AM on July 14, 2020: member

Oh actually this might be fixing undefined behavior according to https://en.cppreference.com/w/cpp/container/span/operator_at
theStack approved
theStack commented at 6:41 PM on July 19, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/19508/commits/fa8a99258947a9ee3749fa472180542920cd471c

nit: Not directly related to this change and maybe more a matter of taste, but I'd always prefer an explicit testing .size() == 0 over an implicit testing .size() < 1 that can only result to true for one value. (Even more clear would be .empty(), but that is not implemented yet in our substitue spans).
span: Add Span::empty() and use it in script/descriptor fa2ae0ac8d
MarcoFalke commented at 6:52 PM on July 19, 2020: member

Fixed in a new commit

Edit: Reference: https://en.cppreference.com/w/cpp/container/span/empty
DrahtBot commented at 12:48 AM on July 20, 2020: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
theStack commented at 12:54 PM on July 20, 2020: member

re-ACK https://github.com/bitcoin/bitcoin/pull/19508/commits/fa2ae0ac8d43086430a29c73940ad6b1cd129e96

in src/span.h:154 in fa2ae0ac8d

 150 | @@ -151,6 +151,7 @@ class Span
 151 |          return m_data[m_size - 1];
 152 |      }
 153 |      constexpr std::size_t size() const noexcept { return m_size; }
 154 | +    constexpr bool empty() const noexcept { return size() == 0; }

jonatack commented at 1:32 PM on July 20, 2020:

Verified this line is being hit by debug-building without it :smile:

script/descriptor.cpp:831:25: error: ‘__gnu_cxx::__alloc_traits<std::allocator<Span<const char> >, Span<const char> >::value_type’
{aka ‘class Span<const char>’} has no member named ‘empty’
  831 |     if (origin_split[0].empty() || origin_split[0][0] != '[') {
      |                         ^~~~~
script/descriptor.cpp:833:43: error: ‘__gnu_cxx::__alloc_traits<std::allocator<Span<const char> >, Span<const char> >::value_type’ 
{aka ‘class Span<const char>’} has no member named ‘empty’
  833 |                           origin_split[0].empty() ? /** empty, implies split char */ ']' : origin_split[0][0]);
      |                                           ^~~~~

jonatack commented at 2:16 PM on July 20, 2020:

Also was good to re-visit spanparsing.h/cpp for this PR.

jonatack commented at 2:04 PM on July 20, 2020: member

ACK fa2ae0ac8d43086430a29c73940ad6b1cd129e96
elichai commented at 8:48 AM on July 26, 2020: contributor

ACK fa2ae0ac8d43086430a29c73940ad6b1cd129e96

I think the standard makes this UB to allow using analysis mechanisms like: https://github.com/bitcoin/bitcoin/blob/fa2ae0ac8d43086430a29c73940ad6b1cd129e96/src/span.h#L157 sanitizers will also probably check bounds here

P.S. see the precondition in the standard's draft: http://eel.is/c++draft/views.span#span.elem-1
sipa commented at 4:27 PM on July 26, 2020: member

utACk
MarcoFalke merged this on Jul 26, 2020
MarcoFalke closed this on Jul 26, 2020
MarcoFalke deleted the branch on Jul 26, 2020
sidhujag referenced this in commit 8376abbe44 on Jul 28, 2020
deadalnix referenced this in commit 210f991f78 on Jun 30, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Work around memory-aliasing in descriptor ParsePubkey #19508

Conflicts