Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Memory leaking in memmove

Reply
Thread Tools

Memory leaking in memmove

 
 
Jens Thoms Toerring
Guest
Posts: n/a
 
      12-28-2011
ArifulHossain tuhin <(E-Mail Removed)> wrote:
> On Wednesday, December 28, 2011 4:36:37 PM UTC+6, Philip Lantz wrote:
> > Are you sure that packet->size + pad_len < sizeof packet->data.buf? That
> > check should probably be in the code, unless there are constraints on
> > the packet size that guarantee that it is always true.


> Packet size can be as high as 230 with pad_len.


You mean that 'packet->size + pad_len' is never larger than 230?
How can that be if, as you claim yourself below, 'pad_len' can be
as large as 255?

> unsigned char pad_len;
> pad_len = (unsigned char)((random() % RANSTOP ) + RANSTART);


> pad_len is unsigned char. so it should cover upto 256.


I guess you mean 255. And then don't cast but do

pad_len = ( random( ) % RANSTOP + RANSTART ) & 0xFF;

Unsigned char must cover at least the range from 0 to 255 but it
can have a larger range (char does not necessarily just have 8
bits).

> if(pad_len%30 == 0)
> pad_len = 9;


This looks rather strange. Why reduce 'pad_len' to 9 only when
it's a multiple of 30?

> if(packet->size > 0){
> memmove(packet->data.buf + pad_len, packet->data.buf, packet->size);


So what exactly are 'pad_len' and 'packet->size' when it segfaults?
If it really crashes at this point then they must be something
different from what you expect them to be.

> packet->size += pad_len;
> packet->data.buf[0] = pad_len;
> for(j=1; j< pad_len; j++)
> packet->data.buf[j] = (char)256%j;


Casting 256 to a char looks rather ridiculous. Typiclally chars
are in the range from 0 to 255 or -128 to 127. What are you at-
tempting to achieve with all this?

> EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);
> }
>
> the packet data structure is decalared as follows:
>
> struct rtp_packet {
> size_t size;
> ................
> (some fields)
> ................
> struct rtp_packet *next;
> struct rtp_packet *prev;
> union {
> rtp_hdr_t header;
> unsigned char buf[8192];
> } data;


> sizeof(packet->data.buf) is 8 so it should not be a problem. Though
> i guess should put a checking into the code anyway.


The size of the buffer 'buf' must be 8192. Where did you get that
8 from?
Regards, Jens
--
\ Jens Thoms Toerring ___ http://www.velocityreviews.com/forums/(E-Mail Removed)
\__________________________ http://toerring.de
 
Reply With Quote
 
 
 
 
ArifulHossain tuhin
Guest
Posts: n/a
 
      12-28-2011
Thank you for everyone's help. i have solved it. apparently i was destorying packet in place where i should not have.
 
Reply With Quote
 
 
 
 
Ben Bacarisse
Guest
Posts: n/a
 
      12-28-2011
ArifulHossain tuhin <(E-Mail Removed)> writes:

> My application is leaking memory in a memmove call. I have made it sure from coredumps.
>
> the offending code snippet reads as follows:
> unsigned char pad_len;
> pad_len = (unsigned char)((random() % RANSTOP ) + RANSTART);
> if(pad_len%30 == 0)
> pad_len = 9;
> if(packet->size > 0){
> memmove(packet->data.buf + pad_len, packet->data.buf, packet->size);
> packet->size += pad_len;
> packet->data.buf[0] = pad_len;
> for(j=1; j< pad_len; j++)
> packet->data.buf[j] = (char)256%j;


Two small points, probably unrelated to your segfault. Why start j at
1? When pad_len > 0 you will leave data.buf[0] as it was, which looks
odd to a casual observer. Secondly, the cast to char does nothing. The
operands of % undergo the usual arithmetic conversions so (char)256 will
be promoted to int which is the type of the literal constant 256.

> EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);
> }


<snip>
--
Ben.
 
Reply With Quote
 
Seebs
Guest
Posts: n/a
 
      12-28-2011
On 2011-12-28, ArifulHossain tuhin <(E-Mail Removed)> wrote:
> The app crashes with seg fault. Coredump's backtrace shows it occurs in memove() statements.


I think you confused us by calling this a "memory leak". Which it's not.

A memory leak is where you allocate memory and lose the pointer to it
so you can't free it.

-s
--
Copyright 2011, all wrongs reversed. Peter Seebach / (E-Mail Removed)
http://www.seebs.net/log/ <-- lawsuits, religion, and funny pictures
http://en.wikipedia.org/wiki/Fair_Game_(Scientology) <-- get educated!
I am not speaking for my employer, although they do rent some of my opinions.
 
Reply With Quote
 
Barry Schwarz
Guest
Posts: n/a
 
      12-28-2011
On Wed, 28 Dec 2011 01:24:59 -0800 (PST), ArifulHossain tuhin
<(E-Mail Removed)> wrote:

>My application is leaking memory in a memmove call. I have made it sure from coredumps.
>
>the offending code snippet reads as follows:
>unsigned char pad_len;
>pad_len = (unsigned char)((random() % RANSTOP ) + RANSTART);


Are you using the cast to suppress an over-zealous diagnostic about
conversion between signed and unsigned or about int to char? If not,
it serves no purpose.

In a subsequent message, you stated RANSTOP was 3 and RANSTART was 9.
random() is not a standard function but if it works like rand() than
the first addend will be in the range of 0 to 2. Even if random()
produces negative values, the range would be -2 to 2. Consequently
the sum must be in the range 9 to 11 (or 7 to 11).

>if(pad_len%30 == 0)


So when will this ever evaluate to true?

>pad_len = 9;


Even without braces, indenting is still a good idea.

>if(packet->size > 0){


What is the initial value of size?

> memmove(packet->data.buf + pad_len, packet->data.buf, packet->size);


Is this the only call to memmove in your program? If this is indeed
the statement causing the seg fault, you need to look at the current
values of pad_len and size. Since pad_len <= 11, size is probably the
real culprit.

> packet->size += pad_len;


If this block is in a loop that does not terminate as expected, it is
quite reasonable for size to grow larger than expected.

> packet->data.buf[0] = pad_len;
> for(j=1; j< pad_len; j++)
> packet->data.buf[j] = (char)256%j;


This would not silence the diagnostic I mentioned above. The division
is performed on int values so (char)256 is promoted. The result is an
int which could generate either the signed->unsigned or int->char
complaints.

Changing it to (char)(256%j) could still lead to the signed->unsigned
diagnostic.

For the cast to have any value, it needs to be (unsigned char)(256%j).
Since the existing code apparently did not cause any of the
diagnostics which would prompt this, I have to assume this cast is
pointless also.

> EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);


I don't know if it matters, but 256%j will generate multiple instances
of 0 in various elements of buf.

>}
>
>the packet data structure is decalared as follows:
>
>struct rtp_packet {
> size_t size;
> ................
> (some fields)
> ................
> struct rtp_packet *next;
> struct rtp_packet *prev;
> union {
> rtp_hdr_t header;
> unsigned char buf[8192];
> } data;
>};
>
>As the data.buf is statically allocated, valgrind does not complain anything about it.
>Is it running out of memory? or any other problem?
>Any suggestion?


--
Remove del for email
 
Reply With Quote
 
Philip Lantz
Guest
Posts: n/a
 
      12-28-2011
Ben Bacarisse wrote:
> ArifulHossain writes:
> > the offending code snippet reads as follows:
> > unsigned char pad_len;
> > pad_len = (unsigned char)((random() % RANSTOP ) + RANSTART);
> > if(pad_len%30 == 0)
> > pad_len = 9;
> > if(packet->size > 0){
> > memmove(packet->data.buf + pad_len, packet->data.buf, packet->size);
> > packet->size += pad_len;
> > packet->data.buf[0] = pad_len;
> > for(j=1; j< pad_len; j++)
> > packet->data.buf[j] = (char)256%j;

>
> Two small points, probably unrelated to your segfault. Why start j at
> 1? When pad_len > 0 you will leave data.buf[0] as it was, which looks
> odd to a casual observer. Secondly, the cast to char does nothing. The
> operands of % undergo the usual arithmetic conversions so (char)256 will
> be promoted to int which is the type of the literal constant 256.


Interesting that you should make these two comments. I had written
both of these comments in my earlier response, but then deleted them
before sending. It turns out they are both wrong.

The loop starts j at 1 because data.buf[0] is set immediately before the
loop.

Your comment on the (char) cast (which was exactly what I would have
written) is true, but on any machine the OP is likely to be using, it
will also convert the value 256 to 0. This means that every byte of the
padding except the first is set to 0.

In the off chance that char is more than 8 bits, the padding is set to
the sequence: 0 0 1 0 1 4 4 0 ... It seems unlikely this is what is
intended.

Philip
 
Reply With Quote
 
Ben Bacarisse
Guest
Posts: n/a
 
      12-28-2011
Philip Lantz <(E-Mail Removed)> writes:

> Ben Bacarisse wrote:
>> ArifulHossain writes:
>> > the offending code snippet reads as follows:
>> > unsigned char pad_len;
>> > pad_len = (unsigned char)((random() % RANSTOP ) + RANSTART);
>> > if(pad_len%30 == 0)
>> > pad_len = 9;
>> > if(packet->size > 0){
>> > memmove(packet->data.buf + pad_len, packet->data.buf, packet->size);
>> > packet->size += pad_len;
>> > packet->data.buf[0] = pad_len;
>> > for(j=1; j< pad_len; j++)
>> > packet->data.buf[j] = (char)256%j;

>>
>> Two small points, probably unrelated to your segfault. Why start j at
>> 1? When pad_len > 0 you will leave data.buf[0] as it was, which looks
>> odd to a casual observer. Secondly, the cast to char does nothing. The
>> operands of % undergo the usual arithmetic conversions so (char)256 will
>> be promoted to int which is the type of the literal constant 256.

>
> Interesting that you should make these two comments. I had written
> both of these comments in my earlier response, but then deleted them
> before sending. It turns out they are both wrong.


So they are.

> The loop starts j at 1 because data.buf[0] is set immediately before the
> loop.


Ah, now that's me just not paying attention.

> Your comment on the (char) cast (which was exactly what I would have
> written) is true, but on any machine the OP is likely to be using, it
> will also convert the value 256 to 0. This means that every byte of the
> padding except the first is set to 0.


How odd. I never considered the value, thinking that it's just another
pointless cast. In a way it probably is pointless but for other
reasons...

> In the off chance that char is more than 8 bits, the padding is set to
> the sequence: 0 0 1 0 1 4 4 0 ... It seems unlikely this is what is
> intended.


No, I doubt it is. Anyway, thanks for paying more attention that I did!

--
Ben.
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Q: stl, boost smart pointer to prevent memory leaking zl2k C++ 6 04-03-2006 12:39 PM
Leaking memory when writing to URL Simon Andrews Java 6 01-06-2006 02:53 PM
Memory leaking Sigmathaar C++ 6 12-20-2005 04:47 PM
Could this C extension be leaking memory? Sam L. Python 0 09-24-2005 07:32 PM
huge recursive and memory leaking? jojo Java 5 09-08-2005 11:39 AM



Advertisments