[Home]

Summary:ASTERISK-23508: Memory Corruption in __ast_string_field_ptr_build_va
Reporter:Arnd Schmitter (arnd)Labels:
Date Opened:2014-03-20 07:44:54Date Closed:2014-08-11 05:39:30
Priority:MajorRegression?
Status:Closed/CompleteComponents:Utilities/General
Versions:11.8.0 Frequency of
Occurrence		Frequent
Related
Issues:
Environment:Centos 6.5 / x86_64Attachments:( 0) issueA23508_stringfieldptr_corruption2.patch
Description:We had serveral cases of memory corruption which occured inside this function, which resulted in random segmentation faults.

We have already found the cause of the corruption:

It happens when the variable space, at the start of the functions calculates to a value of 0.
When the variable "available" later gets calculated, it will make a underrun and because its unsigned, the value gets very high.
So the vsnprintf will always think there are enough bytes and write over the borders of the allocated memory area.




Comments:By: Matt Jordan (mjordan) 2014-03-25 09:56:21.796-0500

Since you believe you have found the source of the corruption, do you have a possible solution in mind? If so, a patch would probably help speed up any issue resolution.
By: Arnd Schmitter (arnd) 2014-03-26 01:44:58.019-0500

Before i can upload a patch i would first need to sign a license agreement and for this i must talk with our legal deparment. But on the other hand we would like to see the problem fixed ;)

There is only a little fix needed. Below is part of the official code. If you look at the else case, there you can see that available is space minus alignment size, If space is < alignment size, available would get negative  (if it wasn't a unsigned). So I just put a if .. then .. else structure arround the instruction and set available = 0 if the calculation would get negative.

{code}
       if (*ptr != __ast_string_field_empty) {
               target = (char *) *ptr;
               available = AST_STRING_FIELD_ALLOCATION(*ptr);
               if (*ptr == mgr->last_alloc) {
                       available += space;
               }
       } else {
               /* pool->used is always a multiple of ast_alignof(ast_string_field_allocation)
                * so we don't need to re-align anything here.
                */
               target = (*pool_head)->base + (*pool_head)->used + ast_alignof(ast_string_field_allocation);
              available = space - ast_alignof(ast_string_field_allocation);
       }
{code}

The corruption was fixed on my system by the way. Itis now running for a week now without any corruptions.

By: Walter Doekes (wdoekes) 2014-03-26 03:23:54.340-0500

I attached {{issueA23508_stringfieldptr_corruption.patch}} based on your observations.

Instead of adding an extra if, I switched to signed size_t's, both for available and needed, which both could get negative. Also I added an extra check for when vsnprintf returns negative.

This should probably fix your issue. Please confirm. And thanks for the analysis :)
By: Arnd Schmitter (arnd) 2014-03-26 04:20:31.805-0500

I don't think, that this will fix the corruption problem.

available can now get negative, but when the vnsprintf function gets called, it will be converted to an unsigned again. Look at the function declaration of vnsprinft, The second parameter is of type size_t.

In my opinion, it would be better to explicit set available to 0 if it is negative, before the call to vnsprinft.

By: Walter Doekes (wdoekes) 2014-03-26 04:36:50.320-0500

Oh. You are absolutely right. I was a bit quick there.
By: Walter Doekes (wdoekes) 2014-03-26 05:18:18.408-0500

Bah. utils.c is riddled with implicit casts and truncations. (See -Wconversion).

Try this new patch.
By: Arnd Schmitter (arnd) 2014-03-26 05:30:43.075-0500

The patch looks good. I will give it a try and see if any problems are arising.
By: Arnd Schmitter (arnd) 2014-04-01 02:49:19.460-0500

The patch works. There weren't any problems so far.
By: JoshE (n8ideas) 2014-08-08 08:17:48.471-0500

We've also seen this issue and the patch does seem to resolve.