Trang chủ‎ > ‎RCE‎ > ‎Linux diassembly‎ > ‎

Debugging an invalid memory read

Debugging an invalid memory read

by Patrick Horgan

It seemed like the right thing to do!

Click to show Table of Contents

Why'd we write this code anyway?

Working on a web server in C++, (I really was working on an adaptive thread pool class, but there's a lot of stuff to test web servers for scalability, so it was easy enough to write a minimal web server), I'd wrapped sockets that talked to clients in a class that would let me insert things exactly as you would on a std::ostream.

There's one problem with inserters though, they can't return error codes.

They have a signature something like

inline sockfdwrapper& operator<<(sockfdwrapper& sfw,const char *msg) { sfw.sendall(msg,strlen(msg)); return sfw; }

They always return their first argument so that you can chain, something like

sfd << "the info: " << info << '\n';

So what do if there are errors? Writing data into a socket connected to a browser often fails. If the user clicks onto another page, the socket isn't connected to anything anymore.

One choice is to keep state about validity in the sockfdwrapper, and have every inserter check to see if the sockfdwrapper is still valid. If it's not, you'd just return the first argument.

inline sockfdwrapper& operator<<(sockfdwrapper& sfw,const char *msg) { if(!sfw.is_valid()){ return sfw; } sfw.sendall(msg,strlen(msg)); return sfw; }

That's kind of a pain though. It is a great idea to keep validity information in the wrapper, and I do that, but it doesn't fit well with inserters. Instead, I decided to throw an exception when something went wrong.

I figured it would be easy enough to make an exception derived from std::exception. It would take errno as an argument when something went wrong and pass information back to anyone who wanted to catch it. The standard what() method would let them ask for information.

class socket_insert_fail: virtual public std::exception { public: socket_insert_fail(int err):err(err){}; virtual ~socket_insert_fail() throw() {}; int error() const { return err; }; virtual const char* what() const throw() { std::stringstream ss; ss << "Couldn't insert into a socket, " << strerror_r(err,const_cast<char*>(the_buf),1023); return ss.str().c_str(); }; private: int err; char the_buf[1024]; };

A std::stringstream lets you insert anything into it, and then use the str() method to get a std::string back. The std::string has a method, c_str() which returns a reference to a regular null terminated string which we need for the return value of what(). The signature of what(), virtual const char* what() const throw() is from the parent class, std::exception.

So now valgrind sticks its nose in.

So I'd been testing scalability, and fixed some bugs and decided to run valgrind against the server. valgrind is a tool that is used for, among other things, to find out about memory leaks. I got this:

==5450== Thread 3: ==5450== Invalid read of size 1 ==5450== at 0x402901A: strlen (mc_replace_strmem.c:282) ==5450== by 0x40FFE9D: std::basic_ostream<char, std::char_traits<char> >& std::operator<< <std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*) (in /usr/lib/i386i-linux-gnu/libstdc++.so.6.0.16) ==5450== by 0x804CE62: one_request(void*) (httpserver.cpp:445) ==5450== by 0x8050773: waitAndRun(void*) (adaptiveThreadPool.cpp:29) ==5450== by 0x4061D30: start_thread (pthread_create.c:304) ==5450== by 0x42520CD: clone (clone.S:130) ==5450== Address 0x4329f7c is 12 bytes inside a block of size 55 free'd ==5450== at 0x4027919: operator delete(void*) (vg_replace_malloc.c:387) ==5450== by 0x410BC4A: std::string::_Rep::_M_destroy(std::allocator<char> const&) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.16) ==5450== by 0x804C4FC: send_file(sockfdwrapper&, http_request_line&, std::map<std::string, std::string, std::less<std::string>, std::allocator<std::pair<std::string const, std::string> > >&) (httpserver.cpp:365) ==5450== by 0x804CE62: one_request(void*) (httpserver.cpp:445) ==5450== by 0x8050773: waitAndRun(void*) (adaptiveThreadPool.cpp:29) ==5450== by 0x4061D30: start_thread (pthread_create.c:304) ==5450== by 0x42520CD: clone (clone.S:130)

The thing to notice is that we made an invalid read, it was by basic_ostream, so we're trying to write something, and that something was previously freed when a std::string was destroyed. It was freed by line 365 in httpserver.cpp. That line is the one shown in the catch block below that tries to insert the return value of sif.what() into std::cerr.

}catch(socket_insert_fail sif){ std::cerr << sif.what() << '\n'; }

So what happened? The problem is in the return value from what(),

return ss.str().c_str();

The signature of stringstream::str() is:

basic_string<charT,traits,Allocator> str() const;

so it returns a string which will be local to our what() method. Then we call c_str() on the string, which has signature:

const charT* c_str() const noexcept;

i.e. it returns a pointer into a data area of the string. I'm sure you can see it now, what() returns the pointer into a data area of the string, but the string is destroyed when it goes out of scope when what() returns! If C++ was a garbage collected language, there'd still be a reference and we'd be ok, but it's not.

So what should we do? I see three choices.

  1. Give up on the idea of a meaningful what()
  2. Make a data area in the socket_insert_fail and let what() add a string or a character buffer there.
  3. Let the constructor build the string that what() will return

Give up on the idea of a meaningful what()

This is viable. We give them a way to get the original errno number, and they could generate the string themselves within their own local scope. It's a shame though that every place that does this has to in essence duplicate the code we'd remove.

Make a data area in the socket_insert_fail and let what() add a string or a character buffer there.

If we did that, what() couldn't set the data to anything, because its signature declares that what() is const, i.e. it won't change the class in any way.

If we remove the const from the declaration of what, then, since that is part of the signature, what() won't be recognized as being a replacement for exception's what, so catch blocks that catch exception will not get the output of our what(), but of exception's what. If we remove the const, we might as well remove the virtual also. While we're at it, we could just make our what return a string() if we wanted.

Perversely, catching as a const socket_insert_fail& will fail, since we will call a non-const method.

That would be ok, it we want to make it a requirement that the catch block explicitly catches our class, they have to do that to get the error number anyway. So, this would be viable.

One good thing about doing it this way, is that resources might by freed before what() is called so we're less likely to have other problems that would lead to an exception in our what().

Let the constructor build the string that what() will return

This is also a great idea, with the caveat that we are more likely to throw an alloc exception when we create the string if resources are getting tight. Our main reason for throwing this will be because the other end of the communication has closed their end of the socket, but it can also happen if memory has gotten tight. Another disadvantage of this is that each exception has to pay the penalty of string construction even if the client doesn't use it.

And the winner is...

I'm going with #3! Here's the new definition:

class socket_insert_fail: public std::exception { public: socket_insert_fail(int err):err(err){ std::stringstream ss; ss << "Couldn't insert into a socket, " << strerror_r(err,const_cast<char*>(the_buf),1023); the_str=ss.str(); }; virtual ~socket_insert_fail() throw() {}; int error() const { return err; }; virtual const char* what() const throw() { return the_str.c_str(); }; private: int err; std::string the_str; char the_buf[1024]; };

Now we create a temporary stringstream in the constructor, and assign a copy of its string to the_str. The lifetime of the_str is the same as for the exception, so it is safe to return the result of the_str.c_str() from what();

Comments