[CRIU] [PATCH 1/7] util: new string helpers introduced

[Stanislav Kinsburskiy]

16.12.2015 18:44, Pavel Emelyanov пишет:
> On 12/16/2015 06:33 PM, Stanislav Kinsburskiy wrote:
>> This patch brings add_to_string() and construct_string() helpers.
>> They allow to create a string with variable amount of parameters in sprintf()
>> manner, but supporting string allocation (and reallocation if necessary)
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
>> ---
>>   include/util.h |    4 ++++
>>   util.c         |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 65 insertions(+)
>>
>> diff --git a/include/util.h b/include/util.h
>> index df708c8..5030065 100644
>> --- a/include/util.h
>> +++ b/include/util.h
>> @@ -275,4 +275,8 @@ void tcp_nodelay(int sk, bool on);
>>   void tcp_cork(int sk, bool on);
>>   
>>   const char *ns_to_string(unsigned int ns);
>> +
>> +char *add_to_string(char *str, const char *fmt, ...);
>> +char *construct_string(const char *fmt, ...);
>> +
>>   #endif /* __CR_UTIL_H__ */
>> diff --git a/util.c b/util.c
>> index d56f973..a27b0b6 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -51,6 +51,67 @@
>>   
>>   #define VMA_OPT_LEN	128
>>   
>> +static char *add_to_string_vargs(char *str, const char *fmt, va_list args)
>> +{
>> +	size_t offset = 0, delta;
>> +	int ret;
>> +	char *new;
>> +	va_list tmp;
>> +
>> +	if (str)
>> +		offset = strlen(str);
>> +	delta = strlen(fmt) * 2;
>> +
>> +	do {
>> +		ret = -ENOMEM;
>> +		new = xrealloc(str, offset + delta);
>> +		if (new) {
>> +			va_copy(tmp, args);
>> +			ret = vsnprintf(new + offset, delta, fmt, tmp);
>> +			if (ret >= delta) {
>> +				delta = ret +1;
>> +				str = new;
>> +				ret = 0;
>> +			}
>> +		}
>> +	} while (ret == 0);
>> +
>> +	if (ret == -ENOMEM) {
>> +		/* realloc failed. We must release former string */
>> +		pr_err("Failed to allocate string\n");
>> +		xfree(str);
>> +	} else if (ret < 0) {
>> +		/* vsnprintf failed */
>> +		pr_err("Failed to print string\n");
>> +		xfree(new);
>> +		new = NULL;
> When this routine returns NULL it's undefined whether the original
> string is freed or not, so called is likely to leak memory.

Sorry, but I don't understand, where it happens (or maybe don't 
understand, what do you mean).

If ret == -ENOMEM, it means realloc returned NULL.
 From man page of realloc:
"If realloc() fails, the original block is left untouched; it is not 
freed or moved"
Thus, previous allocation have to be freed.

If ret < 0, but !ENOMEM, then it means, that realloc succeeded, but 
vsnprintf failed for some reason. And thus newly reallocated string have 
to be freed.
Another piece from realloc man page:
"The realloc() function changes the size of the memory block pointed to 
by ptr to size bytes".
Thus freeing of "new" includes freeing of "str".

IOW, add_to_string always _modifies_ the the passed str pointer.. If it 
failed, then the original string if freed is lost.

A few examples to illustrate:

Example #1 (wrong one!!!):

char *buf = "text";
char *str;
int nr = 10;

str = add_to_string(buf, " about %d hobbits", nr);

This is wrong, because (from man page):
"Unless ptr is NULL, it must have been returned by an earlier call to 
malloc(), calloc() or realloc().", while buf is located on stack

Example #2 (risky usage!!!):

char *buf;
char *str;
int nr = 10;

buf = malloc(10);
strcpy(buf, "text");

str = add_to_string(buf, " about %d hobbits", nr);

In this example, if add_to_string() returned NULL, then buf is also 
freed. This have to be taken into account.

Example #3 (correct):

char *buf;
char *str;
int nr = 10;

buf = malloc(10);
strcpy(buf, "text");

str = add_to_string(NULL, "%s about %d hobbits", buf, nr);

Example #4 (also correct):

char *buf;
char *str = NULL;
int nr = 10;

buf = malloc(10);
strcpy(buf, "text");

str = add_to_string(str, "%s about %d hobbits", buf, nr);

Example #5 (also correct):

char *buf;
char *str;
int nr = 10;

buf = malloc(10);
strcpy(buf, "text");

buf = malloc(10);
strcpy(buf, "This is ");

str = add_to_string(str, "%s about %d hobbits", buf, nr);

>> +	}
>> +	return new;
>> +}
>> +
>> +char *add_to_string(char *str, const char *fmt, ...)
>> +{
>> +	va_list args;
>> +
>> +	va_start(args, fmt);
>> +	str = add_to_string_vargs(str, fmt, args);
>> +	va_end(args);
>> +
>> +	return str;
>> +}
>> +
>> +char *construct_string(const char *fmt, ...)
>> +{
>> +	va_list args;
>> +	char *str;
>> +
>> +	va_start(args, fmt);
>> +	str = add_to_string_vargs(NULL, fmt, args);
>> +	va_end(args);
>> +
>> +	return str;
>> +}
>> +
>>   static void vma_opt_str(const struct vma_area *v, char *opt)
>>   {
>>   	int p = 0;
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU at openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>> .
>>