MySQL Bugs: #26859: String::shrink does not check realloc's failure, is it safe?

Bug #26859	String::shrink does not check realloc's failure, is it safe?
Submitted:	6 Mar 2007 8:15	Modified:	2 Apr 2008 16:09
Reporter:	Shan Lu	Email Updates:
Status:	Not a Bug	Impact on me:	None
Category:	MySQL Server: General	Severity:	S3 (Non-critical)
Version:	5.1-BK, 5.0-BK, 5.2.0-falcon-alpha	OS:	Linux (Linux)
Assigned to:	Kristofer Pettersson	CPU Architecture:	Any

Description:
In the String::shrink function (line 203 in sql/sql_string.h), if my_realloc does not succeed, realloc will be called. However, the shrink function does not check the return value of realloc and simply finish the shrink execution. Isn't this dangerous?

inline void shrink(uint32 arg_length)         // Shrink buffer
  {
    if (arg_length < Alloced_length)
    {
      char *new_ptr;
      if (!(new_ptr=(char*) my_realloc(Ptr,arg_length,MYF(0))))
      {
        Alloced_length = 0;
        real_alloc(arg_length);
      }
      else{
       ..
      }
     }
  }

In above code, once allocation fails inside real_alloc, String object's Ptr and alloced are still the same as prior to the shrink call, however, Alloced_length becomes 0. I don't know what would happen afterwards, but just these three variables' memory states are inconsistent. And no following code is aware of the real_alloc failure.

How to repeat:
no test case. got through source code checking

Thank you for a problem report. As real_alloc() function can really return TRUE or FALSE:

bool String::real_alloc(uint32 arg_length)
{
  arg_length=ALIGN_SIZE(arg_length+1);
  str_length=0;
  if (Alloced_length < arg_length)
  {
    free();
    if (!(Ptr=(char*) my_malloc(arg_length,MYF(MY_WME))))
      return TRUE;
    Alloced_length=arg_length;
    alloced=1;
  }
  Ptr[0]=0;
  return FALSE;
}

the result should be checked in all cases (I think). Verified by code review.

But realloc() cannot fail, why do we have a branch for it at all ?

Sergey, please elaborate why real_alloc can not fail.

  if (!(point = malloc(size)))
  {
...

  if (my_flags & MY_FAE+MY_WME)
      my_error(EE_OUTOFMEMORY, MYF(ME_BELL+ME_WAITTANG),size);

Still I believe it's not a bug since the String object itself is left in _consistent_ state (no memory is allocated, Alloced_length is 0.

I mean, realloc (or my_realloc) cannot fail, not real_alloc.
The code is

 if (arg_length < Alloced_length)
 {
   if (!(new_ptr=(char*) my_realloc(Ptr,arg_length,MYF(0))))
   {
     Alloced_length = 0;
     real_alloc(arg_length);
   }

but as the second if() condition can never be true, real_alloc() is in the dead code which is never executed. And can be removed completely.

The shrink function is dangerous if you use q_append or similar functions which doesn't check allocated memory. It isn't dangerous for append which checks the allocation boundaries using Alloced_length, which is indeed set to 0 on failure.

I'm not following the reasoning above on realloc, real_alloc and dead code. Where is the bug?

No bug here, the code is safe.

The bug report says that return value from real_alloc() in shrink() isn't checked. There is no need to do that as real_alloc() is guaranteed to return FALSE (success). Indeed, we have

  inline void shrink(uint32 arg_length)		// Shrink buffer
  {
    if (arg_length < Alloced_length)
     {
        ...
	real_alloc(arg_length);

And in real_alloc():

bool String::real_alloc(uint32 arg_length)
{
  ...
  if (Alloced_length < arg_length)
  {
    free();
    if (!(Ptr=(char*) my_malloc(arg_length,MYF(MY_WME))))
    ...
  }
  Ptr[0]=0;
  return FALSE;
}

That is, *no* memory allocation will be attempted in real_alloc() if Alloced_length >= arg_length, which is the only case when real_alloc() can be called in shrink().