Bug 398569

Summary:	invalid reads reported in libarmmem memcmp when using strings
Product:	[Developer tools] valgrind	Reporter:	Bjorn <bjorn.vantilt>
Component:	memcheck	Assignee:	Paul Floyd <pjfloyd>
Status:	CONFIRMED ---
Severity:	normal	CC:	bjorn.vantilt, pjfloyd
Priority:	NOR
Version First Reported In:	3.13.0
Target Milestone:	---
Platform:	unspecified
OS:	Unspecified
Latest Commit:		Version Fixed/Implemented In:
Sentry Crash Report:
Attachments:	Valgrind log Patch for libarmmem

Description Bjorn 2018-09-13 09:17:10 UTC

Created attachment 114928 [details]
Valgrind log

Uname -a : Linux raspPDS5 4.14.34-v7+ #1110 SMP Mon Apr 16 15:18:51 BST 2018 armv7l GNU/Linux

Valgrind running on a raspberry pi 3 reporting invalid reads of size 8 in the library libarmmem.so. Some investigation later it is revealed that it is in the function memcpy. It happens on concatenating of two strings. Link to stack overflow question https://stackoverflow.com/questions/52270106/valgrind-reporting-invalid-read-with-stdstring. Some code where it happens: 

/** log message */
void Book::record(std::string file, const int line, const unsigned int level, Identifier id, const std::string message,
                  const std::chrono::high_resolution_clock::time_point timeStamp)
{
    if (!(fileLevels & level) && !(consoleLevels & level)) { return; }

    auto now = Time::keeper->now();
    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(timeStamp - Time::globalEpoch);

    //generate message
    auto entry = std::make_unique<Entry>(level);

    // Time since startup
    addField(entry, 0, std::to_string(duration.count()));

    //UTC Time
    addField(entry, 1, now.dateTime());

    // File
    std::string stringFile;
    if (!file.empty())
    {
        stringFile = URL{file}.lastPathComponent();
    }
    addField(entry, 2, stringFile);

    //Line number
    addField(entry, 3, std::to_string(line));

    //ID
    addField(entry, 4, id);

    //Message
    std::string stringMessage;
    if(!message.empty())
    {
        addField(entry, 5, message); //this is line LogBook.cpp:87
    }
    else
    {
        addField(entry, 5, " empty message.");
    }
    *entry << ";";

    //queue message
    this->append(std::move(entry));
}
void Book::addField(std::unique_ptr<Entry> &entry, unsigned int index, const std::string &text)
{
    std::string textOutput;

    if ((spacings.at(index) != 0) && (text.length() > (spacings.at(index) - 1)))
    {
        spacings.at(index) = (uint8_t) (text.length() + 2);
    }

    entry->setWidth(spacings.at(index));

    if(entry->empty())
        textOutput = text;
    else
        textOutput = ";" + text;   //This is line LogBook.cpp:149

    if(!textOutput.empty())
        (*entry) << textOutput;
}

Comment 1 Julian Seward 2019-03-10 07:12:21 UTC

This happens because the partial-loads-ok heuristic inside Memcheck
only applies to word-sized loads on arm32, not to double-word-sized
loads, as would be required here.  It *might* be possible to make it
do so since IIRC the mips32 port does support p-l-ok on 64 bit loads.

Comment 2 Paul Floyd 2024-01-23 20:33:10 UTC

Scratching my head a bit as I gave it a quick try and it didn't work - shouldn't there be redirs for these functions in shared/vg_replace_strmem.c

paulf@raspberrypi:~/scratch/valgrind $ nm -D /usr/lib/arm-linux-gnueabihf/libarmmem-v8l.so
000004d0 T memcmp
00000b58 T memcpy
0000216c T memmove
000030e4 T __mempcpy
000030e4 T mempcpy
000030f8 T memset
000031c0 T strlen

Comment 3 Paul Floyd 2024-01-26 12:30:09 UTC

Well the libarmmem.so library has a NULL soname. At the moment I think that is causing the debuginfo detection to fail.

Comment 4 Paul Floyd 2024-01-26 19:18:57 UTC

--- Reading (ELF, standard) dynamic symbol table (14 entries) ---
raw symbol [   1]: LOC SEC : svma 0x0000000374, sz    0  NONAME
raw symbol [   2]: LOC SEC : svma 0x0000014024, sz    0  NONAME
raw symbol [   3]: WEA FUN : svma 0x0000000000, sz    0  __cxa_finalize
    ignore -- size=0: __cxa_finalize
raw symbol [   4]: WEA NOT : svma 0x0000000000, sz    0  _ITM_deregisterTMCloneTable
raw symbol [   5]: WEA NOT : svma 0x0000000000, sz    0  __gmon_start__
raw symbol [   6]: WEA NOT : svma 0x0000000000, sz    0  _ITM_registerTMCloneTable
raw symbol [   7]: GLO NOT : svma 0x00000030f8, sz    0  memset
raw symbol [   8]: GLO NOT : svma 0x00000030e4, sz    0  mempcpy
raw symbol [   9]: GLO NOT : svma 0x00000031c0, sz    0  strlen
raw symbol [  10]: GLO NOT : svma 0x000000216c, sz    0  memmove
raw symbol [  11]: GLO NOT : svma 0x00000030e4, sz    0  __mempcpy
raw symbol [  12]: GLO NOT : svma 0x0000000b58, sz    0  memcpy
raw symbol [  13]: GLO NOT : svma 0x00000004d0, sz    0  memcmp

------ Canonicalising the acquired info ------

------ Notifying m_redir ------

------ name = /usr/lib/arm-linux-gnueabihf/libarmmem-v7l.so
------ end ELF OBJECT -------------------------------------------------------

Explains it all. No type. No size.

Comment 5 Paul Floyd 2024-01-27 08:57:27 UTC

I've added an issue to the raspbian bug tracker along with an initial patch

https://bugs.launchpad.net/raspbian/+bug/2051392

Comment 6 Paul Floyd 2024-01-27 12:21:29 UTC

Created attachment 165272 [details]
Patch for libarmmem

Patch to redirect mem/str functions in libarmmem

Comment 7 Paul Floyd 2024-03-16 19:47:43 UTC

I've submitted a pull request here

https://github.com/bavison/arm-mem/pull/12

Comment 8 Paul Floyd 2024-03-24 05:53:19 UTC

And the pull request has been merged.