Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer

[Fan Du <fan.du@windriver.com>]

On 2013年01月15日 20:42, Neil Horman wrote:
> On Tue, Jan 15, 2013 at 09:28:42AM +0800, Fan Du wrote:
>>
>>
>> On 2013年01月14日 22:16, Neil Horman wrote:
>>> On Mon, Jan 14, 2013 at 05:45:43PM +0800, Fan Du wrote:
>>>>
>>>>
>>>> On 2013年01月14日 16:40, Daniel Borkmann wrote:
>>>>> On 01/14/2013 03:37 AM, Fan Du wrote:
>>>>>> CLIENT repeatly call process_ready_sockets, which malloc without free,
>>>>>> so sctp_xconnect exit unexpectly. Since SERVER and CLIENT could share
>>>>>> one buffer, so we malloc an global buffer at start.
>>>>>>
>>>>>> Signed-off-by: Fan Du<fan.du@windriver.com>
>>>>>> ---
>>>>>> apps/sctp_xconnect.c | 19 ++++++++-----------
>>>>>> 1 files changed, 8 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/apps/sctp_xconnect.c b/apps/sctp_xconnect.c
>>>>>> index 5874c33..5be5a34 100644
>>>>>> --- a/apps/sctp_xconnect.c
>>>>>> +++ b/apps/sctp_xconnect.c
>>>>>> @@ -73,6 +73,7 @@ char *remote_host = NULL;
>>>>>> sockaddr_storage_t client_loop,
>>>>>> server_loop;
>>>>>> struct hostent *hst;
>>>>>> +char *big_buffer;
>>>>>>
>>>>>> void usage(char *argv0);
>>>>>> void parse_arguments(int argc, char*argv[]);
>>>>>> @@ -380,13 +381,8 @@ void server_mode() {
>>>>>> int assoc_num =0;
>>>>>> struct msghdr inmessage;
>>>>>> struct iovec iov;
>>>>>> - char *big_buffer;
>>>>>> char incmsg[CMSG_SPACE(sizeof(sctp_cmsg_data_t))];
>>>>>>
>>>>>> - if ((big_buffer = malloc(REALLY_BIG)) = NULL) {
>>>>>> - printf("malloc failure: %s\n", strerror(errno));
>>>>>> - DUMP_CORE;
>>>>>> - }
>>>>>>
>>>>>> printf("Running in Server Mode...\n");
>>>>>>
>>>>>> @@ -530,15 +526,9 @@ void process_ready_sockets(int client_socket[], int assoc_num, fd_set *rfds) {
>>>>>> int i, stream, error;
>>>>>> struct msghdr inmessage;
>>>>>> struct iovec iov;
>>>>>> - char *big_buffer;
>>>>>> char incmsg[CMSG_SPACE(sizeof (sctp_cmsg_data_t))];
>>>>>> sockaddr_storage_t msgname;
>>>>>>
>>>>>> - if ((big_buffer = malloc(REALLY_BIG)) = NULL) {
>>>>>> - printf("malloc failure: %s\n", strerror(errno));
>>>>>> - DUMP_CORE;
>>>>>> - }
>>>>>> -
>>>>>> /* Setup inmessage to be able to receive in incomming message */
>>>>>> memset(&inmessage, 0, sizeof (inmessage));
>>>>>> iov.iov_base = big_buffer;
>>>>>> @@ -579,11 +569,18 @@ int main(int argc, char *argv[]) {
>>>>>>
>>>>>> parse_arguments(argc, argv);
>>>>>>
>>>>>> + if ((big_buffer = malloc(REALLY_BIG)) = NULL) {
>>>>>> + printf("malloc failure: %s\n", strerror(errno));
>>>>>> + DUMP_CORE;
>>>>>> + }
>>>>>> +
>>>>>
>>>>> This is still not what Neil meant in his feedback. Please fix it and resend
>>>>> your patch.
>>>>>
>>>>
>>>> Oh, This malloced size is about 64K bytes, I don't think static declare 64k bytes buffer is good for a quite few codes.
>>>> And also, as I have stated, SERVER and CLIENT could share the same buffer, so malloc this space at startup for one time
>>>> is easy for the rest to use.
>>>>
>>> Daniel is right.  While this is again a fine solution, but suggestion was to
>>> statically declare the buffer in a global location.  It provides the same shared
>>> usage between client and server modes, and doesn't require the NULL checking.
>>
>> Yes, no NULL pointer checking, that's true.
>>
>>> understand its a large buffer, but I'm not sure in what situation a statically
>>> declared 64k buffer would be inadvisable whereas a heap allocated 64k buffer
>>> would be acceptable, at least not in a bit of code so small as this.
>>
>> Hmmm, Ok, I will send another patch to follow your suggestion by declaring it statically :)
>>
>> But sorry, I have to argue this a bit more privately, because I'm not convinced why it should do that way.
>> This statically declared buffer contains nothing before its running, not like a CRC32
>> table which is a MUST to declare statically, so why bother to declare it statically to
>> make the executable file size explode considering its few code size.

crap! I regret once I sent this out.....


>>
> Well, 64k isn't that big, and it won't increase the size of the exutable by that
> anyway.  If you don't initalize the global variable, then it will be placed in
> the bss section of the elf file, where it is stored as just an offset and a
> distance.  Give it a try and check the resultant binary size.
>
> This doesn't make sense for all situations, but given the small size of this
> program, I think you will find you will actually reduce your program size, as
> the disk storage for a global uninitalized variable will be smaller than the
> amount of storage for the code you remove in the NULL checking.

You are right, I forgot the uninitialized variable sits in bss.



> Neil
>
>>
>>
>>> Neil
>>>
>>>>
>>>>>> if (mode = SERVER) {
>>>>>> server_mode();
>>>>>> } else if (mode = CLIENT){
>>>>>> client_mode();
>>>>>> }
>>>>>> +
>>>>>> + free (big_buffer);
>>>>>> exit(1);
>>>>>> }
>>>>>>
>>>>>>
>>>>
>>>> --
>>>> 浮沉随浪只记今朝笑
>>>>
>>>> --fan
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>
>> --
>> 浮沉随浪只记今朝笑
>>
>> --fan
>>

-- 
浮沉随浪只记今朝笑

--fan