uninitialised value & valgrind

Discussion in 'C Programming' started by Mark, Jun 26, 2012.

  1. Mark

    Mark Guest

    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
     
    Mark, Jun 26, 2012
    #1
    1. Advertising

  2. Mark

    James Kuyper Guest

    On 06/26/2012 12:35 PM, Mark wrote:
    > 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.
     
    James Kuyper, Jun 26, 2012
    #2
    1. Advertising

  3. Mark

    Mark Guest

    "James Kuyper" <> wrote in message
    news:...
    [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
     
    Mark, Jun 26, 2012
    #3
  4. Mark

    James Kuyper Guest

    On 06/26/2012 02:07 PM, Mark wrote:
    > "James Kuyper" <> wrote in message
    > news:...
    > [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..
     
    James Kuyper, Jun 26, 2012
    #4
  5. Mark wrote:
    > 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.
    > Thanks.
    >
    > Mark
    >
    >
     
    Johann Klammer, Jun 27, 2012
    #5
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Andy Fish
    Replies:
    7
    Views:
    555
    David Carlisle
    Jan 10, 2005
  2. Cognition Peon

    use of uninitialised value

    Cognition Peon, Apr 15, 2004, in forum: Perl Misc
    Replies:
    5
    Views:
    152
    Ala Qumsieh
    Apr 15, 2004
  3. Len
    Replies:
    3
    Views:
    194
  4. Jess
    Replies:
    2
    Views:
    176
  5. Justin C
    Replies:
    6
    Views:
    142
    Justin C
    Jul 23, 2010
Loading...

Share This Page