uninitialised value & valgrind

M

Mark

Hello

I'm working with a legacy code that I link to my application, and run it
under 'valgrind' tool to catch memory leaks, corrupted memory etc. So here
is what it looks like:

#define VLOG_MSG_MAX_DATA_SIZE 1024
struct vlog_msg_header
{
u_int32_t vmh_vr_id;
u_int16_t vmh_msg_type;
module_id_t vmh_mod_id;
u_int32_t vmh_proc_id;
s_int8_t vmh_priority;
u_int32_t vmh_data_len;
};

struct vlog_msg
{
struct vlog_msg_header vms_msg_hdr;
char vms_msg_data[VLOG_MSG_MAX_DATA_SIZE+1];
};

int vlog_client_send_message (struct vlog_msg *msg)
{
...
write (sock, (void *) msg, msg_len); /* line 103. */
}

int vlog_client_send_ctrl_msg ()
{
struct vlog_msg_header header;

/* initialise 'header' .. */

vlog_client_send_message ((struct vlog_msg *)&header); /* line 41*/

return RES_OK;
}

Valgrind reports:

==1634== Syscall param write(buf) points to uninitialised byte(s)
==1634== at 0x6D2D22E: __write_nocancel (syscall-template.S:82)
==1634== by 0x8116E68: vlog_client_send_message (vlog_client.c:103)
==1634== by 0x8116EBF: vlog_client_send_ctrl_msg (vlog_client.c:41)
==1634== by 0x8116EE6: vlog_client_connect_cb (vlog_client.c:211)
==1634== by 0x80C9A3F: message_client_start (message.c:597)
==1634== by 0x8116BA8: vlog_client_start (vlog_client.c:294)
==1634== by 0x8116DA1: vlog_client_create (vlog_client.c:255)
==1634== by 0x80C8FF1: openzlog (log.c:116)
==1634== by 0x805AF95: ldp_start (ldp_main.c:85)
==1634== by 0x8049F39: main (ldp.c:167)
==1634== Address 0xfe541aee is on thread 1's stack
==1634== Uninitialised value was created by a stack allocation
==1634== at 0x8116E7F: vlog_client_send_ctrl_msg (vlog_client.c:32)

I assume valgrind complains because vlog_client_send_message() expects to
get object of type 'struct vlog_msg', but instead we pass 'struct
vlog_msg_header' and everything beyond it (what is supposed to be occupied
by 'char vms_msg_data[VLOG_MSG_MAX_DATA_SIZE+1]' is undefined. Does it sound
correct ?

My question is -- is it valid to typecast this way or it involves undefined
behaviours?
Thanks.

Mark
 
J

James Kuyper

Hello

I'm working with a legacy code that I link to my application, and run it
under 'valgrind' tool to catch memory leaks, corrupted memory etc. So here
is what it looks like:

#define VLOG_MSG_MAX_DATA_SIZE 1024
struct vlog_msg_header
{
u_int32_t vmh_vr_id;
u_int16_t vmh_msg_type;
module_id_t vmh_mod_id;
u_int32_t vmh_proc_id;
s_int8_t vmh_priority;
u_int32_t vmh_data_len;
};

struct vlog_msg
{
struct vlog_msg_header vms_msg_hdr;
char vms_msg_data[VLOG_MSG_MAX_DATA_SIZE+1];
};

int vlog_client_send_message (struct vlog_msg *msg)
{
...
write (sock, (void *) msg, msg_len); /* line 103. */
}

int vlog_client_send_ctrl_msg ()
{
struct vlog_msg_header header;

/* initialise 'header' .. */

vlog_client_send_message ((struct vlog_msg *)&header); /* line 41*/

return RES_OK;
}

Valgrind reports:

==1634== Syscall param write(buf) points to uninitialised byte(s)
==1634== at 0x6D2D22E: __write_nocancel (syscall-template.S:82)
==1634== by 0x8116E68: vlog_client_send_message (vlog_client.c:103)
==1634== by 0x8116EBF: vlog_client_send_ctrl_msg (vlog_client.c:41)
==1634== by 0x8116EE6: vlog_client_connect_cb (vlog_client.c:211)
==1634== by 0x80C9A3F: message_client_start (message.c:597)
==1634== by 0x8116BA8: vlog_client_start (vlog_client.c:294)
==1634== by 0x8116DA1: vlog_client_create (vlog_client.c:255)
==1634== by 0x80C8FF1: openzlog (log.c:116)
==1634== by 0x805AF95: ldp_start (ldp_main.c:85)
==1634== by 0x8049F39: main (ldp.c:167)
==1634== Address 0xfe541aee is on thread 1's stack
==1634== Uninitialised value was created by a stack allocation
==1634== at 0x8116E7F: vlog_client_send_ctrl_msg (vlog_client.c:32)

I assume valgrind complains because vlog_client_send_message() expects to
get object of type 'struct vlog_msg', but instead we pass 'struct
vlog_msg_header' and everything beyond it (what is supposed to be occupied
by 'char vms_msg_data[VLOG_MSG_MAX_DATA_SIZE+1]' is undefined. Does it sound
correct ?

My question is -- is it valid to typecast this way or it involves undefined
behaviours?

Your code has undefined behavior. vlog_client_send_msg() expects to
receive a pointer to an actual vlog_msg struct, and is entitled to
access msg->vms_msg_data, which in your case doesn't exist. This is
called "lying to the compiler", and behavior such as you describe is a
normal consequence of lying about such things.

The messages you're getting from valgrind suggest that something is
attempting to access the non-existent message data inside of __write().
You don't show us how msg_len is calculated. However, if it's greater
than sizeof(struct vlog_msg_header), that would explain the messages
from valgrind.

I'd recommend figuring out why msg_len Is greater than that limit - it
suggests a serious disconnect between what you think you're doing and
what you're actually doing. The most plausible kind of mistake would be
failing to set header.vmh_data_len to 0.
 
M

Mark

[skip]
The messages you're getting from valgrind suggest that something is
attempting to access the non-existent message data inside of __write().
You don't show us how msg_len is calculated. However, if it's greater
than sizeof(struct vlog_msg_header), that would explain the messages
from valgrind.

I'd recommend figuring out why msg_len Is greater than that limit - it
suggests a serious disconnect between what you think you're doing and
what you're actually doing. The most plausible kind of mistake would be
failing to set header.vmh_data_len to 0.

#define VLOG_MSG_HDR_SIZE sizeof (struct vlog_msg_header)
int vlog_client_send_message (struct vlog_msg *msg)
{
u_int16_t msg_len = VLOG_MSG_HDR_SIZE + msg->vms_msg_hdr.vmh_data_len;
...
}

and this is how 'header' is initialised:

struct vlog_msg_header header;
...
header.vmh_msg_type = VLOG_MSG_TYPE_CTRL;
header.vmh_vr_id = 0;
header.vmh_mod_id = zg->protocol;
header.vmh_proc_id = pal_get_process_id();
header.vmh_priority = 0;
header.vmh_data_len = 0;
vlog_client_send_message (zg->vlog_clt, (struct vlog_msg *)&header);
...

So msg_len should be of valid length.

Mark
 
J

James Kuyper

[skip]
The messages you're getting from valgrind suggest that something is
attempting to access the non-existent message data inside of __write().
You don't show us how msg_len is calculated. However, if it's greater
than sizeof(struct vlog_msg_header), that would explain the messages
from valgrind.

I'd recommend figuring out why msg_len Is greater than that limit - it
suggests a serious disconnect between what you think you're doing and
what you're actually doing. The most plausible kind of mistake would be
failing to set header.vmh_data_len to 0.

#define VLOG_MSG_HDR_SIZE sizeof (struct vlog_msg_header)
int vlog_client_send_message (struct vlog_msg *msg)
{
u_int16_t msg_len = VLOG_MSG_HDR_SIZE + msg->vms_msg_hdr.vmh_data_len;
...
}

and this is how 'header' is initialised:

struct vlog_msg_header header;
...
header.vmh_msg_type = VLOG_MSG_TYPE_CTRL;
header.vmh_vr_id = 0;
header.vmh_mod_id = zg->protocol;
header.vmh_proc_id = pal_get_process_id();
header.vmh_priority = 0;
header.vmh_data_len = 0;
vlog_client_send_message (zg->vlog_clt, (struct vlog_msg *)&header);
...

So msg_len should be of valid length.

I'd recommend checking to make sure that msg_len is still <= sizeof
(struct vlog_msg_header) at the point of the call to write(). It might
have changed, and if so, you need to find out why.

Another possibility is that struct vlog_msg_header may have padding
bytes. The fact that there's an s_int8_t member between two u_int32_t
members would generally result in padding on any system where u_int32_t
has a non-trivial alignment requirement. If that's the case, then those
padding bytes would be uninitialized by your code, but the call to
write() would attempt to write them anyway.

If you have the power to change the structure definitions, I recommend
putting the members in decreasing order of size; this will tend to
minimize the number of padding bytes needed. Note: you can safely
convert from a pointer to a struct and a pointer to the first member of
the struct, and vice versa. If any of the code that uses this struct
takes advantage of that fact, you can't move that member.

There's no way to guarantee that a struct has no padding. The only way
to guarantee initialization of the whole struct, including padding
bytes, is to give the struct object static or thread storage duration,
or to use memset() or something equivalent to memset..
 
J

Johann Klammer

Mark said:
Hello

I'm working with a legacy code that I link to my application, and run it
under 'valgrind' tool to catch memory leaks, corrupted memory etc. So here
is what it looks like:

#define VLOG_MSG_MAX_DATA_SIZE 1024
struct vlog_msg_header
{
u_int32_t vmh_vr_id;
u_int16_t vmh_msg_type;
module_id_t vmh_mod_id;
u_int32_t vmh_proc_id;
s_int8_t vmh_priority;
u_int32_t vmh_data_len;
};

struct vlog_msg
{
struct vlog_msg_header vms_msg_hdr;
char vms_msg_data[VLOG_MSG_MAX_DATA_SIZE+1];
};

int vlog_client_send_message (struct vlog_msg *msg)
{
...
write (sock, (void *) msg, msg_len); /* line 103. */
You may have to check the return value to make sure all data was sent.
If not, then you may have to resume. If you are using GNU libc, take a
look at TEMP_FAILURE_RETRY

nbytes = TEMP_FAILURE_RETRY (write (desc, buffer, count));

man 2 write sez:
[SNIP]
write() writes up to count bytes from the buffer pointed buf to the file
referred to by the file descriptor fd.

The number of bytes written may be less than count if, for
example, there is insufficient space on the underlying physical
medium, or the RLIMIT_FSIZE resource limit is encountered (see
setrlimit(2)), or the call was interrupted by a signal handler
after having written less than count bytes. (See also pipe(7).)
[SNIP]
}

int vlog_client_send_ctrl_msg ()
{
struct vlog_msg_header header;
You almost certainly need a struct vlog_msg here.

/* initialise 'header' .. */

vlog_client_send_message ((struct vlog_msg *)&header); /* line 41*/

return RES_OK;
}

Valgrind reports:

==1634== Syscall param write(buf) points to uninitialised byte(s)
==1634== at 0x6D2D22E: __write_nocancel (syscall-template.S:82)
==1634== by 0x8116E68: vlog_client_send_message (vlog_client.c:103)
==1634== by 0x8116EBF: vlog_client_send_ctrl_msg (vlog_client.c:41)
==1634== by 0x8116EE6: vlog_client_connect_cb (vlog_client.c:211)
==1634== by 0x80C9A3F: message_client_start (message.c:597)
==1634== by 0x8116BA8: vlog_client_start (vlog_client.c:294)
==1634== by 0x8116DA1: vlog_client_create (vlog_client.c:255)
==1634== by 0x80C8FF1: openzlog (log.c:116)
==1634== by 0x805AF95: ldp_start (ldp_main.c:85)
==1634== by 0x8049F39: main (ldp.c:167)
==1634== Address 0xfe541aee is on thread 1's stack
==1634== Uninitialised value was created by a stack allocation
==1634== at 0x8116E7F: vlog_client_send_ctrl_msg (vlog_client.c:32)

I assume valgrind complains because vlog_client_send_message() expects to
get object of type 'struct vlog_msg', but instead we pass 'struct
vlog_msg_header' and everything beyond it (what is supposed to be occupied
by 'char vms_msg_data[VLOG_MSG_MAX_DATA_SIZE+1]' is undefined. Does it sound
correct ? Yes

My question is -- is it valid to typecast this way or it involves undefined
behaviours?
Yes... undef behaviour. Of course some smart ass programmer may have
relied on a specific stack layout, so it may have worked as long as no
one put another variable declaration after the header decl.
But I do not anderstand why you would want to send
VLOG_MSG_MAX_DATA_SIZE of uninitialized stack data around.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,479
Members
44,899
Latest member
RodneyMcAu

Latest Threads

Top