[linux-lvm] [patch] bug in lvm_remove_recursive.c

[linux-lvm] [patch] bug in lvm_remove_recursive.c

[Jan Niehusmann]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

Hello!

I found a malloc size off-by-one bug in lvm_remove_recursive.c.
file_name is allocated with size strlen (dir) + strlen (dir_ent->d_name) + 2,
and then used with sprintf ( file_name, "%s/%s%c", dir, dir_ent->d_name, 0);
As sprintf automatically appends another 0 byte, the allocated memory is one
byte too short.

Normaly this doesn't hurt, but today, after I created a lv with the
relatively long name 'reisertest', I got a segmentation fault in vgscan.
Applying the attached patch cured that. 

Please note that I simply increase the malloc size. Now file_name is 
terminated by two 0 bytes. It may be better to remove the manually added
0, or to switch to snprintf (snprintf doesn't add a 0 byte, IIRC).

Jan



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 608 bytes --]

--- 0.8final/tools/lib/lvm_remove_recursive.c	Tue Feb 22 03:09:32 2000
+++ 0.8final-changed/tools/lib/lvm_remove_recursive.c	Fri Aug  4 11:07:04 2000
@@ -44,7 +44,7 @@
                     strcmp ( dir_ent->d_name,"..") == 0) continue;
                if ( ( file_name = malloc ( strlen (dir) +
                                            strlen (dir_ent->d_name) +
-                                           2)) == NULL) {
+                                           3)) == NULL) {
                   ret = -LVM_EREMOVE_RECURSIVE_MALLOC;
                   goto lvm_remove_recursive_end;
                }

Re: [linux-lvm] [patch] bug in lvm_remove_recursive.c

[Andreas Dilger]

Jan, you write:
> I found a malloc size off-by-one bug in lvm_remove_recursive.c.
> file_name is allocated with size strlen (dir) + strlen (dir_ent->d_name) + 2,
> and then used with sprintf ( file_name, "%s/%s%c", dir, dir_ent->d_name, 0);
> As sprintf automatically appends another 0 byte, the allocated memory is one
> byte too short.

You should look at the BUGS file at the LVM download site.  It fixes this
bug and several more.  Why Heinz doesn't simply include these fixes into
the source and make a new release, I don't know.

> Please note that I simply increase the malloc size. Now file_name is 
> terminated by two 0 bytes. It may be better to remove the manually added
> 0, or to switch to snprintf (snprintf doesn't add a 0 byte, IIRC).

Yes, I noticed in many places that there is an extra '\0' added on to
the end of strings.  I don't understand why this is done.

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert