Buffer overflow in SharedAllocationRecord in Kokkos_HostSpace.cpp

[glhenni]

I've run across what I believe is a bug in Kokkos. Specifically in the function within Kokkos_HostSpace.cpp

SharedAllocationRecord< Kokkos::HostSpace , void >::
SharedAllocationRecord( const Kokkos::HostSpace & arg_space
                      , const std::string       & arg_label
                      , const size_t              arg_alloc_size
                      , const SharedAllocationRecord< void , void >::function_type arg_dealloc
                      )

In that function there is an invocation of strncpy() as follows:

  strncpy( RecordBase::m_alloc_ptr->m_label
          , arg_label.c_str()
          , SharedAllocationHeader::maximum_label_length
          );

The problem occurs when the source string, arg_label, is longer than SharedAllocationHeader::maximum_label_length. Of course strncpy() does what it's supposed to do and only copies the first SharedAllocationHeader::maximum_label_length chars out of arg_label.c_str(), but what it doesn't do is null terminate the c string pointed to by RecordBase::m_alloc_ptr->m_label when arg_label is longer than the maximum. Later on another member function, get_label(), is invoked to return the string but it creates a value to return as a std::string using a ctor that takes m_label above as an argument. Unfortunately that particular std::string ctor assumes it's argument, in this case m_label, is null terminated. Since it's not in the case I describe here an overflow occurs.

One possible fix would be:

  strncpy( RecordBase::m_alloc_ptr->m_label
          , arg_label.c_str()
          , SharedAllocationHeader::maximum_label_length - 1
          );

  RecordBase::m_alloc_ptr->m_label[SharedAllocationHeader::maximum_label_length - 1] = '\0';

But I'm not fully aware of what the consequences of this "fix" might be on other parts of the code.

[crtrott]

This appears to be a valid fix though maybe it should just set it to 0 not the character.

[glhenni]

The other valid way that I'm aware of to set a character to nul is

RecordBase::m_alloc_ptr->m_label[SharedAllocationHeader::maximum_label_length - 1] = (char) 0;

My pure C is rusty though so maybe there are even more ways!

[crtrott]

Fix in #1677