The thing is, snprintf always NULL-terminates its output string, no
matter whether the input was NULL-terminated or not.
However, I looked at the snprintf man page again and found this
description for %s :
> If no l modifier is present: The const char * argument is expected to be
> a pointer to an array of character type (pointer to a string).
> Characters from the array are written up to (but not including) a
> terminating null byte ('\0'); if a precision is specified, no more than
> the number specified are written. If a precision is given, no null byte
> need be present; if the precision is not specified, or is greater than the
> size of the array, the array must contain a terminating null byte.
So apparently "the array must contain a terminating null byte" unless
a precision is specified like %10s, etc.
So you're absolutely right, and the patch looks fine.
Colin
On Fri, Sep 10, 2010 at 5:55 AM, Jim Meyering <jim@xxxxxxxxxxxx> wrote:
>
> * server/msg.c (msg_get): Copy only name_len bytes, then NUL-terminate,
> rather than using snprintf to copy up to and including nonexistent NUL.
> ---
>
> valgrind exposed this. The use of snprintf would have been
> correct if the inode name buffer (following the struct raw_inode)
> were NUL-terminated, but it is not.
>
> Invalid read of size 1
> at 0x3502647FF7: vfprintf (vfprintf.c:1593)
> by 0x350266EFB1: vsnprintf (vsnprintf.c:120)
> by 0x350264F022: snprintf (snprintf.c:35)
> by 0x4061D5: msg_get (msg.c:451)
> by 0x407FD9: udp_rx_handle (server.c:164)
> by 0x408244: udp_rx (server.c:233)
> by 0x4091AA: udp_srv_event (server.c:640)
> by 0x409DE6: main_loop (server.c:1026)
> by 0x40A17E: main (server.c:1135)
> Address 0x4d2afae is 0 bytes after a block of size 62 alloc'd
> at 0x4A0515D: malloc (vg_replace_malloc.c:195)
> by 0x3505F35527: __os_umalloc (os_alloc.c:68)
> by 0x3505EF8F8D: __db_retcopy (db_ret.c:124)
> by 0x3505EF90EB: __db_ret (db_ret.c:69)
> by 0x3505ED93A7: __dbc_iget (db_cam.c:1111)
> by 0x3505EE5CB3: __db_get (db_iface.c:779)
> by 0x3505EE5FDA: __db_get_pp (db_iface.c:694)
> by 0x4040F6: cldb_inode_get (cldb.c:537)
> by 0x406069: msg_get (msg.c:430)
> by 0x407FD9: udp_rx_handle (server.c:164)
> by 0x408244: udp_rx (server.c:233)
> by 0x4091AA: udp_srv_event (server.c:640)
> by 0x409DE6: main_loop (server.c:1026)
> by 0x40A17E: main (server.c:1135)
>
> Here's a fix:
>
>
> server/msg.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/server/msg.c b/server/msg.c
> index f2dda59..8abb1e6 100644
> --- a/server/msg.c
> +++ b/server/msg.c
> @@ -448,7 +448,8 @@ void msg_get(struct session *sess, const void *v)
>
> name_len = le32_to_cpu(inode->ino_len);
> inode_name = alloca(name_len + 1);
> - snprintf(inode_name, name_len + 1, "%s", (char *)(inode + 1));
> + memcpy (inode_name, inode + 1, name_len);
> + inode_name[name_len] = 0;
> resp.inode_name = inode_name;
>
> resp.data.data_len = 0;
> @@ -1172,4 +1173,3 @@ err_out_noabort:
> sess_sendresp_generic(sess, resp_rc);
> free(h);
> }
> -
> --
> 1.7.3.rc0.183.gb0497
> --
> To unsubscribe from this list: send the line "unsubscribe hail-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
