[Pkg-shadow-devel] [pkg-shadow-Bugs][313942] new_line gets truncated in merge_group_entries

[pkg-shadow-bugs at alioth.debian.org]

pkg-shadow-Bugs item #313942 was changed at 05/08/2013 23:10 by Nicolas François
You can respond by visiting: 
https://alioth.debian.org/tracker/?func=detail&atid=411478&aid=313942&group_id=30580

>Status: Closed
Priority: 3
Submitted By: Brad Hubbard (badone-guest)
Assigned to: Nobody (None)
Summary: new_line gets truncated in merge_group_entries 
Category: None
Group: None
>Resolution: Fixed


Initial Comment:
In the following code allowance is made for the terminating NULL in new_line_len but not the newline char that is added when the two lines are concatenated. The result is new_line ends up one character short.

    314         /* Concatenate the 2 lines */
    315         new_line_len = strlen (gr1->line) + strlen (gr2->line) +1;
    316         new_line = (char *)malloc ((new_line_len + 1) * sizeof(char*));
    317         if (NULL == new_line) {
    318                 errno = ENOMEM;
    319                 return NULL;
    320         }
    321         snprintf(new_line, new_line_len, "%s\n%s", gr1->line, gr2->line);
    322         new_line[new_line_len] = '\0';

Patch attached.

----------------------------------------------------------------------

>Comment By: Nicolas François (nekral)
Date: 05/08/2013 23:10

Message:
On line 315, +1 is correct. It is the length of the string after concatenation (with the added \n).

During malloc, an extra byte is reserved for the nul-termination, so +1 on line 316 is also correct.

The only remaining problem is that the malloc allocates sizeof(char *) times more than necessary.

on line 321, snprintf will copy/concatenate the strings (without nul termination). The nul termination is added on line 322.

----------------------------------------------------------------------

Comment By: Nicolas François (nekral)
Date: 04/08/2013 15:02

Message:
sizeof(char*) removed also. (but overall, that's what saved it from overflow ;)

Regarding the size provided to snprintf(), I do not see the problem. snprintf() will not be able to copy the terminating nul, but it is added just afterwards.

----------------------------------------------------------------------

Comment By: Christian Perrier (bubulle)
Date: 29/07/2013 10:07

Message:
I committed the original patch. Improved patch would be appreciated

----------------------------------------------------------------------

Comment By: Tomáš Mráz (tmraz-guest)
Date: 29/01/2013 14:05

Message:
The patch is not quite correct. The malloc is sizeof(char*) times more than needed. And the fix should be by changing the snprintf() to use new_line_len+1 which is the number of bytes we would allocate (if there wasn't the overallocation).


----------------------------------------------------------------------

You can respond by visiting: 
https://alioth.debian.org/tracker/?func=detail&atid=411478&aid=313942&group_id=30580