Why it's not thread safety and how to get it thread safety?

Question

There is some function in FCGI called "getRequestContent()". It just gets the data which posted by web browser. So our c++ application is something like a daemon which serves web clients. I think I have some sort of problem with not "thread safety" function:

/**
 * Note this is not thread safe due to the static allocation of the
 * content_buffer.
 */
std::string getRequestContent(const FCGX_Request &request)
{
    char *content_length_str = FCGX_GetParam("CONTENT_LENGTH", request.envp);
    unsigned long content_length = STDIN_MAX;
    if (content_length_str)
    {
        content_length = strtol(content_length_str, &content_length_str, 10);
        if (*content_length_str)
        {
            std::cerr << "Can't Parse 'CONTENT_LENGTH='"
                      << FCGX_GetParam("CONTENT_LENGTH", request.envp)
                      << "'. Consuming stdin up to " << STDIN_MAX << "\n";
        }
        if (content_length > STDIN_MAX)
        {
            content_length = STDIN_MAX;
        }
    }
    else
    {
        content_length =
            0;  // Do not read from stdin if CONTENT_LENGTH is missing
    }
    char *content_buffer = new char[content_length];
    std::cin.read(content_buffer, content_length);
    content_length = std::cin.gcount();
    do
        std::cin.ignore(1024);
    while (std::cin.gcount() == 1024);
    std::string content(content_buffer, content_length);
    delete[] content_buffer;
    return content;
}

Please explain me why it's not thread safety code? What the sort of a problem we have here? How to get it thread safety? :)

Answer 1

The main problem is, like the comment says, that there is a statically allocated buffer. If two threads would work synchronously on that buffer, you would most likely get race conditions, so you have to avoid that.

That means either fixing FCGX_GetParam (which I doubt will be a good idea since it is a third party library) or synchronizing the access to it:

//some common mutex
std::mutex mtx;

std::string getRequestContent(const FCGX_Request &request)
{
  std::string content_length_str;
  {
    lock(mtx); //guard every action on the static buffer with this lock
    char *content_length_cptr = FCGX_GetParam("CONTENT_LENGTH", request.envp);
    content_length_str = content_length_cptr; //copy the content of the buffer
  } //unlock the mutex, you dont work on the buffer hence forth  

  unsigned long content_length = 0;
  if (!content_length_str.empty()) try {
    content_length = boost::lexical_cast<unsigned long>(content_length_str);
    if (content_length > STDIN_MAX)
    {
      content_length = STDIN_MAX;
    }
  }
  catch(boost::bad_lexical_cast const&)
  {
    std::cerr << "Can't Parse 'CONTENT_LENGTH='"
              << content_length_str
              << "'. Consuming stdin up to " << STDIN_MAX << "\n";
    content_length = STDIN_MAX;
  }

  // the rest as it was...
}

Answer 2

char * content_length_str = FCGX_GetParam( "CONTENT_LENGTH", request.envp );

This line can mean two things:

Either you are leaking memory if the function returns a block allocated by alloc/calloc OR the function uses a static buffer. That would not be thread safe. Considering the comment, I'd guess it's the second option.