Bug 5021 - free gets invalid pointer
free gets invalid pointer
Status: RESOLVED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: libsmbclient
3.0.24
Other Linux
: P3 major
: none
Assigned To: Derrell Lipman
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-16 02:05 UTC by Evgeniy Dushistov
Modified: 2007-11-01 16:01 UTC (History)
1 user (show)

See Also:


Attachments
Patch (541 bytes, patch)
2007-10-20 13:44 UTC, Volker Lendecke
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Evgeniy Dushistov 2007-10-16 02:05:57 UTC
When I use fusesmb: http://www.ricardis.tudelft.nl/~vincent/fusesmb/
in particular fusesmb.cache, I always get such message:
fusesmb.cache: *** glibc detected *** ./fusesmb.cache: free(): invalid pointer: 0xbf8f27ec ***,

valgrind show that this libsmbclient problem,
I look at code:
In function smbc_opendir_ctx in libsmbclient.c we have this:
ip_list = NULL;
if (!name_resolve_bcast(MSBROWSE, 1, &ip_list, &count)) {
 SAFE_FREE(ip_list);
 ip_list = &server_addr;

at now ip_list contais stack varible address,
but later it interpreted as heap address
SAFE_FREE(ip_list);

simple patch is:
Index: test/samba-3.0.24/source/libsmb/libsmbclient.c
===================================================================
--- test.orig/samba-3.0.24/source/libsmb/libsmbclient.c
+++ test/samba-3.0.24/source/libsmb/libsmbclient.c
@@ -2601,6 +2601,7 @@ smbc_opendir_ctx(SMBCCTX *context,
                 struct ip_service server_addr;
                 struct user_auth_info u_info;
                 struct cli_state *cli;
+               int do_not_free_ip_list = 0;
 
                if (share[0] != (char)0 || path[0] != (char)0) {
 
@@ -2645,6 +2646,7 @@ smbc_opendir_ctx(SMBCCTX *context,
                         }
 
                         ip_list = &server_addr;
+                       do_not_free_ip_list = 1;
                         count = 1;
                 }
 
@@ -2692,8 +2694,8 @@ smbc_opendir_ctx(SMBCCTX *context,
                                 continue;
                         }
                 }
-
-                SAFE_FREE(ip_list);
+               if (!do_not_free_ip_list)
+                       SAFE_FREE(ip_list);
         } else { 
                 /*
                  * Server not an empty string ... Check the rest and see what
Comment 1 Volker Lendecke 2007-10-20 13:44:02 UTC
Created attachment 2948 [details]
Patch

Can you try the attached patch?
Comment 2 Evgeniy Dushistov 2007-10-22 03:28:59 UTC
(In reply to comment #1)
> Created an attachment (id=2948) [edit]
> Patch
> 
> Can you try the attached patch?
> 

I tried. It works.

But I wonder, why then use server_addr?
Patch like this also works:
--- samba-3.0.24/source/libsmb/libsmbclient.c   2007-02-04 21:59:20.000000000 +0300
+++ samba-3.0.24.mod/source/libsmb/libsmbclient.c       2007-10-22 11:54:46.000000000 +0400
@@ -2598,9 +2598,8 @@
                 int count;
                 int max_lmb_count;
                 struct ip_service *ip_list;
-                struct ip_service server_addr;
                 struct user_auth_info u_info;
-                struct cli_state *cli;
+                struct cli_state *cli; 
 
                if (share[0] != (char)0 || path[0] != (char)0) {
 
@@ -2633,8 +2632,8 @@
                 if (!name_resolve_bcast(MSBROWSE, 1, &ip_list, &count)) {
 
                         SAFE_FREE(ip_list);
-
-                        if (!find_master_ip(workgroup, &server_addr.ip)) {
+                       ip_list = SMB_MALLOC_P(struct ip_service);
+                        if (!find_master_ip(workgroup, &ip_list->ip)) {
 
                                if (dir) {
                                        SAFE_FREE(dir->fname);
@@ -2643,8 +2642,6 @@
                                 errno = ENOENT;
                                 return NULL;
                         }
-
-                        ip_list = &server_addr;
                         count = 1;
                 }
 
@@ -2693,7 +2690,7 @@
                         }
                 }
 
-                SAFE_FREE(ip_list);
+               SAFE_FREE(ip_list);
         } else { 
                 /*
                  * Server not an empty string ... Check the rest and see what


PS

During testing I got such message:

==27586== 334,490 (72 direct, 334,418 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 17
==27586==    at 0x4005718: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==27586==    by 0x4084002: _talloc (talloc.c:183)
==27586==    by 0x40842C8: talloc_named_const (talloc.c:425)
==27586==    by 0x40843F2: _talloc_memdup (talloc.c:1017)
==27586==    by 0x4084468: talloc_strdup (talloc.c:1035)
==27586==    by 0x405A24C: lp_string (loadparm.c:1712)
==27586==    by 0x40D0A84: resolve_name (namequery.c:1278)
==27586==    by 0x4053B75: smbc_opendir_ctx (libsmbclient.c:2740)
==27586==    by 0x8049736: server_listing (cache.c:244)
==27586==    by 0x8049DEE: workgroup_listing_thread (cache.c:360)
==27586==    by 0x459B24AA: start_thread (in /lib/libpthread-2.5.so)
==27586==    by 0x4581987D: clone (in /lib/libc-2.5.so)

and:
==10591== 349,643 (142 direct, 349,501 indirect) bytes in 1 blocks are definitely lost in loss record 9 of 17
==10591==    at 0x4005718: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==10591==    by 0x4083FF2: _talloc (talloc.c:183)
==10591==    by 0x40842B8: talloc_named_const (talloc.c:425)
==10591==    by 0x40844B2: _talloc_zero (talloc.c:1002)
==10591==    by 0x4084524: _talloc_zero_array (talloc.c:1213)
==10591==    by 0x40C0903: init_unistr2 (parse_misc.c:800)
==10591==    by 0x40E6889: rpccli_srvsvc_net_share_enum (cli_srvsvc.c:134)
==10591==    by 0x40530E1: smbc_opendir_ctx (libsmbclient.c:2480)
==10591==    by 0x8049736: server_listing (cache.c:244)
==10591==    by 0x8049DEE: workgroup_listing_thread (cache.c:360)
==10591==    by 0x459B24AA: start_thread (in /lib/libpthread-2.5.so)
==10591==    by 0x4581987D: clone (in /lib/libc-2.5.so)

Comment 3 Jeremy Allison 2007-11-01 16:01:25 UTC
Fixed for 3.2 with vl's patch.
Jeremy.