Memory leaking in memmove

[ArifulHossain tuhin]

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;
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;
};

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?

 

[Nick Keighley]

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;
        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;

};

As the data.buf is statically allocated, valgrind does not complain anything about it.
Is it running out of memory?

well? is it? How do you know you have memory leak?

or any other problem?
Any suggestion?

I can't really see how this leaks. Where does packet come from?

 

[Philip Lantz]

My application is leaking memory in a memmove call. I have made it
sure from coredumps.

I don't think you mean "leaking memory". That term specifically refers
to not freeing memory that was allocated. I'm guessing you mean that
your program is overwriting memory that it shouldn't.

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;
EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);
}

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?

There is no dynamic memory allocation in the fragment you have shown us,
so it can't be running out of memory.

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.

 

[ArifulHossain tuhin]

The app crashes with seg fault. Coredump's backtrace shows it occurs in memove() statements.

The packet comes from network, its a RTP packet embedded in UDP payload.

 

[Philip Lantz]

The app crashes with seg fault. Coredump's backtrace shows it occurs
in memove() statements.

The packet comes from network, its a RTP packet embedded in UDP payload.

packet->size must be out of range. You should check that it is a valid
value. Perhaps it was -1, indicating an error, which was converted to
size_t, yielding a very large number.

 

[Nick Keighley]

The app crashes with seg fault. Coredump's backtrace shows it occurs in memove() statements.

that doesn't sound like a memory leak to me. Print (or use a debugger)
the values of the parameters passed to the memmove(). In fact why not
just use a debugger? A debugger can also be used on a core dump.

The packet comes from network, its a RTP packet embedded in UDP payload.

I meant, how was it allocated.

 

[ArifulHossain tuhin]

that doesn't sound like a memory leak to me.

Sorry for the mistake. Actually its "SegFaulting".

Print (or use a debugger)
the values of the parameters passed to the memmove(). In fact why not
just use a debugger? A debugger can also be used on a core dump.

I have used gdb to print the parameters passed to memove(). But i'm unsure what to do with it. Any suggestion ? what should i do with the parameters?

I meant, how was it allocated.

rtp_packet is allocated dynamically. But the data.buf field is allocated asyou see as static allocation.

 

[ArifulHossain tuhin]

My application is leaking memory in a memmove call. I have made it
sure from coredumps.

I don't think you mean "leaking memory". That term specifically refers
to not freeing memory that was allocated. I'm guessing you mean that
your program is overwriting memory that it shouldn't.

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;
EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);
}

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?

There is no dynamic memory allocation in the fragment you have shown us,
so it can't be running out of memory.

You are absolutely right. I guess i'm misinformed about terminologies. Thanks

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. pad_len is unsigned char. so it should cover upto 256. sizeof(packet->data.buf) is 8 so it should not be a problem. Though i guess should put a checking into the code anyway.

 

[Dr Nick]

My application is leaking memory in a memmove call. I have made it
sure from coredumps.

I don't think you mean "leaking memory". That term specifically refers
to not freeing memory that was allocated. I'm guessing you mean that
your program is overwriting memory that it shouldn't.

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;
EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);
}

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?

There is no dynamic memory allocation in the fragment you have shown us,
so it can't be running out of memory.

You are absolutely right. I guess i'm misinformed about terminologies. Thanks

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. pad_len is unsigned char. so it should cover upto 256. sizeof(packet->data.buf) is 8 so it should not be a problem. Though i guess should put a checking into the code anyway.

How long is your actual packet buffer? - you say above that it's 8 but
you might be misunderstanding the question. If not, then you'll have
problems.

If at any time packet_size + pad_len is longer than the size of
packet->data.buf then you'll be scribbling on memory you don't own.

Can you show us the code that defines and sets packet->data.buf

As packet->data.buf must contain at least as many bytes as the maximum
length of pad_len, can you tell us what RANSTOP and RANSTART are
#define'd to?

It might be worth printing pad_len just before the memmove and seeing
what the value is when it blows up.

 

[ArifulHossain tuhin]

ArifulHossain tuhin wrote:
My application is leaking memory in a memmove call. I have made it
sure from coredumps.
I don't think you mean "leaking memory". That term specifically refers
to not freeing memory that was allocated. I'm guessing you mean that
your program is overwriting memory that it shouldn't.

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;
EnDeCrypt(packet->data.buf, packet->size, key, KEYLEN);
}

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?


There is no dynamic memory allocation in the fragment you have shown us,
so it can't be running out of memory.

You are absolutely right. I guess i'm misinformed about terminologies. Thanks

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. pad_len is unsigned char. so it should cover upto 256. sizeof(packet->data.buf) is 8 so it should not be a problem. Though i guess should put a checking into the code anyway.

How long is your actual packet buffer? - you say above that it's 8 but
you might be misunderstanding the question. If not, then you'll have
problems.

Its given in the original post.Below the code snippet, the structure definition of packet is given there. Its about 8192 bytes long. more than enough to hold multiple copies. I was talking about the data type of packet->data.buf which unsigned char.

If at any time packet_size + pad_len is longer than the size of
packet->data.buf then you'll be scribbling on memory you don't own.

Can you show us the code that defines and sets packet->data.buf

As packet->data.buf must contain at least as many bytes as the maximum
length of pad_len, can you tell us what RANSTOP and RANSTART are
#define'd to?

3 and 9

 

[Jens Thoms Toerring]

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

 

[ArifulHossain tuhin]

Thank you for everyone's help. i have solved it. apparently i was destorying packet in place where i should not have.

 

[Ben Bacarisse]

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>

 

[Seebs]

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

 

[Barry Schwarz]

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?

 

[Philip Lantz]

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

 

[Ben Bacarisse]

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!