* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
@ 2013-01-11 13:48 ` Neil Horman
2013-01-14 2:37 ` Fan Du
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Neil Horman @ 2013-01-11 13:48 UTC (permalink / raw)
To: linux-sctp
On Fri, Jan 11, 2013 at 03:22:41PM +0800, Fan Du wrote:
> CLIENT repeatly call process_ready_sockets, which malloc without free,
> so sctp_xconnect exit unexpectly.
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
> apps/sctp_xconnect.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/apps/sctp_xconnect.c b/apps/sctp_xconnect.c
> index 5874c33..4e33040 100644
> --- a/apps/sctp_xconnect.c
> +++ b/apps/sctp_xconnect.c
> @@ -573,6 +573,7 @@ void process_ready_sockets(int client_socket[], int assoc_num, fd_set *rfds) {
> }
> }
> }
> + free(big_buffer);
> }
>
Definately a leak, but it seems like it would be better solved by making those
buffers static allocations. Its a single threaded app, why do all that malloc
and free in a forever while loop when you don't really need to?
Neil
> int main(int argc, char *argv[]) {
> --
> 1.7.1
>
> --
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
2013-01-11 13:48 ` Neil Horman
@ 2013-01-14 2:37 ` Fan Du
2013-01-14 2:40 ` Fan Du
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Fan Du @ 2013-01-14 2:37 UTC (permalink / raw)
To: linux-sctp
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;
+ }
+
if (mode = SERVER) {
server_mode();
} else if (mode = CLIENT){
client_mode();
}
+
+ free (big_buffer);
exit(1);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
2013-01-11 13:48 ` Neil Horman
2013-01-14 2:37 ` Fan Du
@ 2013-01-14 2:40 ` Fan Du
2013-01-14 8:40 ` Daniel Borkmann
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Fan Du @ 2013-01-14 2:40 UTC (permalink / raw)
To: linux-sctp
On 2013年01月11日 21:48, Neil Horman wrote:
> On Fri, Jan 11, 2013 at 03:22:41PM +0800, Fan Du wrote:
>> CLIENT repeatly call process_ready_sockets, which malloc without free,
>> so sctp_xconnect exit unexpectly.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>> apps/sctp_xconnect.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/apps/sctp_xconnect.c b/apps/sctp_xconnect.c
>> index 5874c33..4e33040 100644
>> --- a/apps/sctp_xconnect.c
>> +++ b/apps/sctp_xconnect.c
>> @@ -573,6 +573,7 @@ void process_ready_sockets(int client_socket[], int assoc_num, fd_set *rfds) {
>> }
>> }
>> }
>> + free(big_buffer);
>> }
>>
> Definately a leak, but it seems like it would be better solved by making those
> buffers static allocations. Its a single threaded app, why do all that malloc
> and free in a forever while loop when you don't really need to?
Hi, Neil
Thanks for your advice.
I have send another version to the list.
> Neil
>
>> int main(int argc, char *argv[]) {
>> --
>> 1.7.1
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
` (2 preceding siblings ...)
2013-01-14 2:40 ` Fan Du
@ 2013-01-14 8:40 ` Daniel Borkmann
2013-01-14 9:45 ` Fan Du
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2013-01-14 8:40 UTC (permalink / raw)
To: linux-sctp
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.
> if (mode = SERVER) {
> server_mode();
> } else if (mode = CLIENT){
> client_mode();
> }
> +
> + free (big_buffer);
> exit(1);
> }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
` (3 preceding siblings ...)
2013-01-14 8:40 ` Daniel Borkmann
@ 2013-01-14 9:45 ` Fan Du
2013-01-14 14:16 ` Neil Horman
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Fan Du @ 2013-01-14 9:45 UTC (permalink / raw)
To: linux-sctp
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.
>> if (mode = SERVER) {
>> server_mode();
>> } else if (mode = CLIENT){
>> client_mode();
>> }
>> +
>> + free (big_buffer);
>> exit(1);
>> }
>>
>>
--
浮沉随浪只记今朝笑
--fan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
` (4 preceding siblings ...)
2013-01-14 9:45 ` Fan Du
@ 2013-01-14 14:16 ` Neil Horman
2013-01-15 1:28 ` Fan Du
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Neil Horman @ 2013-01-14 14:16 UTC (permalink / raw)
To: linux-sctp
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. I
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.
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
` (5 preceding siblings ...)
2013-01-14 14:16 ` Neil Horman
@ 2013-01-15 1:28 ` Fan Du
2013-01-15 12:42 ` Neil Horman
2013-01-16 1:13 ` Fan Du
8 siblings, 0 replies; 10+ messages in thread
From: Fan Du @ 2013-01-15 1:28 UTC (permalink / raw)
To: linux-sctp
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.
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
` (6 preceding siblings ...)
2013-01-15 1:28 ` Fan Du
@ 2013-01-15 12:42 ` Neil Horman
2013-01-16 1:13 ` Fan Du
8 siblings, 0 replies; 10+ messages in thread
From: Neil Horman @ 2013-01-15 12:42 UTC (permalink / raw)
To: linux-sctp
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.
>
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.
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] sctp_xconnect: memory leak when malloc big buffer
2013-01-11 7:22 [PATCH] sctp_xconnect: memory leak when malloc big buffer Fan Du
` (7 preceding siblings ...)
2013-01-15 12:42 ` Neil Horman
@ 2013-01-16 1:13 ` Fan Du
8 siblings, 0 replies; 10+ messages in thread
From: Fan Du @ 2013-01-16 1:13 UTC (permalink / raw)
To: linux-sctp
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
^ permalink raw reply [flat|nested] 10+ messages in thread