tlv.c unitialised memory issue (debugged)

[GoogleCodeExporter]

What steps will reproduce the problem?
1. valgrind
2. manually run ufs_rebuild on some corrupted directory
3.

Here is what i got:
==1950== Use of uninitialised value of size 4
==1950==    at 0x804B549: tlv_unpack (tlv.c:69)
==1950==    by 0x804B0BC: parse_header (rebuild_entry.c:72)
==1950==    by 0x80497C0: read_file (ufs_build_dir.c:88)
==1950==    by 0x8049AF3: rebuild_from_dir (ufs_build_dir.c:148)
==1950==    by 0x80492DA: main (ufs_rebuild.c:86)                         

I add some simple debug
    assert(hdr_len != NULL);

+    debug(20, 0) ("entering tlv_unpack: hdr_len %d!\n", *hdr_len);
#define STORE_META_OK   0x03
    if (buf[j++] != (char) STORE_META_OK)

and

    while (buflen - j >= (sizeof(char) + sizeof(int))) {
+        debug(20, 0) ("tlv_unpack: buflen %d j %d hdr_len %d!\n", buflen,
j,*hdr_len);
        type = buf[j++];

And got:
2009/09/30 03:54:17| entering tlv_unpack: hdr_len 1024!
2009/09/30 03:54:17| tlv_unpack: buflen 1287 j 5 hdr_len 1024!
2009/09/30 03:54:17| tlv_unpack: buflen 1287 j 26 hdr_len 1024!
2009/09/30 03:54:17| tlv_unpack: buflen 1287 j 55 hdr_len 1024!
2009/09/30 03:54:17| tlv_unpack: buflen 1287 j 1274 hdr_len 1024!
(here i got valgrind error. For sure!)

So maybe somewhere
    xmemcpy(&buflen, &buf[j], sizeof(int));
    j += sizeof(int);

must be checked if retrieved buflen is exceeding hdr_len ? or something
like that:

    if (buflen > hdr_len-sizeof(char)-sizeof(int))
            return NULL;

Original issue reported on code.google.com by nuclear...@gmail.com on 30 Sep 2009 at 1:12

[GoogleCodeExporter]

Sorry, sure:
    if (buflen > *hdr_len-sizeof(char)-sizeof(int))
            return NULL;

Original comment by nuclear...@gmail.com on 30 Sep 2009 at 1:15

[GoogleCodeExporter]

Try this patch against libsqtlv/tlv.c :

Index: tlv.c
===================================================================
--- tlv.c       (revision 14309)
+++ tlv.c       (working copy)
@@ -59,6 +59,11 @@
     xmemcpy(&buflen, &buf[j], sizeof(int));
     j += sizeof(int);

+    if (buflen > (*hdr_len) - sizeof(char) - sizeof(int)) {
+        debug(20, 0) ("tlv_unpack: unable to unpack: passed buffer size %d 
bytes; TLV length %d bytes; header prefix size %d 
bytes\n", buflen, *hdr_len, (int) (sizeof(char) + sizeof(int)));
+        return NULL;
+    }
+
     /*
      * sanity check on 'buflen' value.  It should be at least big
      * enough to hold one type and one length.

Original comment by adrian.c...@gmail.com on 2 Oct 2009 at 4:03

• Changed state: Accepted
• Added labels: Version-Head
• Removed labels: Version-1.0

[GoogleCodeExporter]

Yes it is working fine. Here is what i got as example in logs on corrupted 
files:

2009/10/04 02:31:00| tlv_unpack: unable to unpack: passed buffer size 1545 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:06| tlv_unpack: unable to unpack: passed buffer size 1529 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:09| tlv_unpack: unable to unpack: passed buffer size 1333 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:15| tlv_unpack: unable to unpack: passed buffer size 2047 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:25| tlv_unpack: unable to unpack: passed buffer size 1333 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:29| tlv_unpack: unable to unpack: passed buffer size 1022 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:58| tlv_unpack: unable to unpack: passed buffer size 1174 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes
2009/10/04 02:31:59| tlv_unpack: unable to unpack: passed buffer size 2044 
bytes; TLV
length 1024 bytes; header prefix size 5 bytes

Original comment by nuclear...@gmail.com on 3 Oct 2009 at 11:32

[GoogleCodeExporter]

committed, revision r14313. thanks!

Original comment by adrian.c...@gmail.com on 4 Oct 2009 at 1:57

• Changed state: Fixed