Author Topic: Using uninitialized memory (valgrind report)  (Read 15895 times)

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12073
    • Travis build status
Using uninitialized memory (valgrind report)
« on: October 16, 2013, 10:18:17 am »
Here is what valgrind have spit http://pastebin.com/qscKUNt8
How to reproduce:
1. build
2. cd src && ./update
3. cd devel && export LD_LIBRARY_PATH=`pwd`
4. valgrind ./codeblocks -p debug
5. open codeblocks-unix.cbp

I'm not too familiar with the wxString's implementation, but the function mentioned in the reports is full of constructs that look suspicious.
Examples:
Code: [Select]
while (*ptr <= ParserConsts::space_chr)
    ++ptr;
...
while (*(ptr+1) == ParserConsts::ptr_chr)
{
     baseArgs << *ptr; // append one more '*' to baseArgs
     ptr++; // next char
}
...
// skip white spaces and increase pointer
while (   *ptr     != ParserConsts::null
          && *(ptr+1) == ParserConsts::space_chr )
{
      ++ptr; // next char
}

I'm not familiar with this code, so it will be good if someone could look this up.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #1 on: October 16, 2013, 10:43:21 am »
Can you tell me what is context of your test?
Did you use a 64 bit Linux? or 32 bit Linux? --->looking at your log, it looks like you are using 64bit.
32 bit wxWidgets or 64 bit wxWidgets library?
Did you use 2.8.12 version or 2.9.x version of wxWidgets?
I believe that the wxString implementation has changed from 2.8 -> 2.9+, this means using a raw pointer(usually wxChar *) will cause problems. Different system now use different kind of wxString.
See my related post here:http://forums.codeblocks.org/index.php/topic,18353.msg125462.html#msg125462
« Last Edit: October 16, 2013, 10:44:58 am by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #2 on: October 16, 2013, 11:15:21 am »
Ok, besides the potential issue about wxString in 2.9.x+, I found that the issue reported is definitely related to an unsafe pointer access.

Look at the code snippet:

Code: [Select]
bool ParserThread::GetBaseArgs(const wxString& args, wxString& baseArgs)
{
    const wxChar* ptr = args;  // pointer to current char in args string
    wxString word;             // compiled word of last arg
    bool skip = false;         // skip the next char (do not add to stripped args)
    bool sym  = false;         // current char symbol
    bool one  = true;          // only one argument

    TRACE(_T("GetBaseArgs() : args='%s'."), args.wx_str());
    baseArgs.Alloc(args.Len() + 1);

    // Verify ptr is valid (still within the range of the string)
    while (*ptr != ParserConsts::null)
    {
        switch (*ptr)
        {
        case ParserConsts::eol_chr:
            while (*ptr <= ParserConsts::space_chr)   // this the the line parserthread.cpp:2859 reported by valgrind
                ++ptr;
            break;
        case ParserConsts::space_chr:
        ...

You see, when we meet an "eol_chr", we are going to run a loop:
Code: [Select]
            while (*ptr <= ParserConsts::space_chr)   // this the the line parserthread.cpp:2859 reported by valgrind
                ++ptr;

You see, we don't check whether we have meet a "ParserConsts::null" in the loop, so I think the simple fix is adding this check. Any ideas?

If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12073
    • Travis build status
Re: Using uninitialized memory (valgrind report)
« Reply #3 on: October 16, 2013, 12:12:28 pm »
Gentoo linux 64bit, wxGTK 2.8.12, Codeblocks 9399 or something like this.

If you look at my post you'll see that all code excerpts are from this function.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #4 on: October 16, 2013, 04:40:24 pm »
Gentoo linux 64bit, wxGTK 2.8.12, Codeblocks 9399 or something like this.
Ok, I see.

Quote
If you look at my post you'll see that all code excerpts are from this function.
Yes, there are many "Conditional jump or move depends on uninitialised value(s)" reports from valgrind, the reason is that after the pointer passes the eof (the null character at the end of the string, it becomes Dangling pointer.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12073
    • Travis build status
Re: Using uninitialized memory (valgrind report)
« Reply #5 on: October 16, 2013, 05:18:45 pm »
Can they be fixed?
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #6 on: October 17, 2013, 01:51:19 am »
Can they be fixed?
Sure, the main point here is that avoid using a raw pointer. In wx 2.9.x+ under Linux, the UTF8 is used to store characters, so we should use index to access each element of the wxString.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #7 on: October 17, 2013, 05:23:29 pm »
the main point here is that avoid using a raw pointer. In wx 2.9.x+ under Linux, the UTF8 is used to store characters, so we should use index to access each element of the wxString.
Ok, a little off topic, I see that the new wxString now use fixed width encoding since 2012-05-13, so using a raw pointer(wchar_t *) is OK in all systems. See my post: Re: wxString support in wxWidgets 3.0 problem?
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12073
    • Travis build status
Re: Using uninitialized memory (valgrind report)
« Reply #8 on: October 17, 2013, 06:25:17 pm »
I don't know if it is OK, because I'm not too familiar with wxString in both 2.8 and 2.9, but what I'm sure is that all these constructs in my first post are broken and dangerous.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #9 on: October 18, 2013, 06:42:22 am »
...but what I'm sure is that all these constructs in my first post are broken and dangerous.
Yes, the bug in first post should be fixed.
I just debugged a little under wx2.8.12, and see that the wxString is always terminated by Null characters, see the figures and document in the webpage: http://docs.wxwidgets.org/trunk/overview_string.html
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #10 on: October 18, 2013, 07:00:37 am »
Code: [Select]
src/plugins/codecompletion/parser/parserthread.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/plugins/codecompletion/parser/parserthread.cpp b/src/plugins/codecompletion/parser/parserthread.cpp
index b614aca..221b548 100644
--- a/src/plugins/codecompletion/parser/parserthread.cpp
+++ b/src/plugins/codecompletion/parser/parserthread.cpp
@@ -2856,7 +2856,7 @@ bool ParserThread::GetBaseArgs(const wxString& args, wxString& baseArgs)
         switch (*ptr)
         {
         case ParserConsts::eol_chr:
-            while (*ptr <= ParserConsts::space_chr)
+            while (*ptr != ParserConsts::null && *ptr <= ParserConsts::space_chr)
                 ++ptr;
             break;
         case ParserConsts::space_chr:
@@ -2876,7 +2876,7 @@ bool ParserThread::GetBaseArgs(const wxString& args, wxString& baseArgs)
         case ParserConsts::ptr_chr: // handle pointer args
             // handle multiple pointer like in: main (int argc, void** argv)
             // or ((int *, char ***))
-            while (*(ptr+1) == ParserConsts::ptr_chr)
+            while (*(ptr+1) != ParserConsts::null && *(ptr+1) == ParserConsts::ptr_chr)
             {
                 baseArgs << *ptr; // append one more '*' to baseArgs
                 ptr++; // next char

Does the patch above fix the issue reported by valgrind? (I just add some null check when accessing by pointer de-reference)
Thanks.
« Last Edit: October 20, 2013, 08:20:03 am by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5234
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Using uninitialized memory (valgrind report)
« Reply #11 on: October 22, 2013, 03:16:39 am »
Committed in r9416, I hope it fix the bug.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.