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 j...@meyering.net 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:)
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 majord...@vger.kernel.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
