Bug 7045 - Bad (non memory copying) interfaces in smbc_setXXXX calls.
Bad (non memory copying) interfaces in smbc_setXXXX calls.
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: libsmbclient
unspecified
Other All
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-15 19:07 UTC by Jeremy Allison
Modified: 2010-01-18 03:29 UTC (History)
1 user (show)

See Also:
derrell.lipman: review+


Attachments
Patch to clean up these interfaces to be malloc-safe. (3.21 KB, patch)
2010-01-15 19:18 UTC, Jeremy Allison
no flags Details
Complete patch for master. (3.28 KB, patch)
2010-01-15 19:36 UTC, Jeremy Allison
jra: review? (gd)
jra: review? (derrell.lipman)
Details
git-am format patch for 3.5.0. (4.07 KB, patch)
2010-01-15 19:47 UTC, Jeremy Allison
no flags Details
git-am format patch for 3.4.5. (4.06 KB, patch)
2010-01-15 19:51 UTC, Jeremy Allison
no flags Details
git-am fix for 3.3.11. (4.05 KB, patch)
2010-01-15 19:54 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2010-01-15 19:07:08 UTC
From Günther Deschner:

I think libsmbclient is
really doing something wrong here: in smbc_free_context libsmbclient just
calls free() on the string options so it assumes the callers have malloced
them before setting them via smbc_set calls.

From Jeremy Allison:

libsmbclient should have been deep copying the strings from the start,
not just stealing the pointers.

Can we track down the gvfs backend code that calls this
and see how hard it would be to fix this mess ?

Ok, that latest git code I can find for gvfs both say:

http://git.gnome.org/browse/gvfs/tree/daemon/gvfsbackendsmb.c
http://git.gnome.org/browse/gvfs/tree/daemon/gvfsbackendsmbbrowse.c

  /* FIXME: is strdup() still needed here? -- removed */
  if (default_workgroup != NULL)
    smbc_setWorkgroup (smb_context, default_workgroup);

So it looks like we really need to make this a memory
duplicating interface asap and fix the calling code
in smbc_init_context().
Comment 1 Jeremy Allison 2010-01-15 19:18:00 UTC
Created attachment 5191 [details]
Patch to clean up these interfaces to be malloc-safe.

Please review this fix for master. If you concur Guenther and can confirm it's safe for gvfs I'll back-port for 3.5.0, 3.4.5 and 3.3.11.

Jeremy.
Comment 2 Jeremy Allison 2010-01-15 19:20:03 UTC
Comment on attachment 5191 [details]
Patch to clean up these interfaces to be malloc-safe.

Adding Derrell to the review list.
Comment 3 Jeremy Allison 2010-01-15 19:36:27 UTC
Created attachment 5192 [details]
Complete patch for master.

Complete patch, includes the fixes that went into master as :

2d41b1ab78639abe4ae030ff482573f464564dd7

and

f85b6ee90b88c7f7b2a92c8a5f3e2ebe59c1087b
Comment 4 Jeremy Allison 2010-01-15 19:47:49 UTC
Created attachment 5193 [details]
git-am format patch for 3.5.0.
Comment 5 Jeremy Allison 2010-01-15 19:51:28 UTC
Created attachment 5194 [details]
git-am format patch for 3.4.5.
Comment 6 Jeremy Allison 2010-01-15 19:54:02 UTC
Created attachment 5195 [details]
git-am fix for 3.3.11.
Comment 7 Volker Lendecke 2010-01-16 05:09:01 UTC
Looks entirely right. Karolin, please put this into the relevant branches.

Thanks,

Volker
Comment 8 Volker Lendecke 2010-01-16 05:11:30 UTC
BTW, while this is definitely the right thing to do, it might create memleaks in correct current users of the current API. Or did I get something wrong?

Volker
Comment 9 Jeremy Allison 2010-01-16 14:32:14 UTC
The only current user of the API I could find is Gnome gvfs, which explicitly *removed* the strdup they had in older code. So right now they'll probably end up with a crash, and I'm ok with memleaking "workgroup", "netbiosname" and "username" in older code which strdup'ed it, for the sake of ending up with a sane interface. No other way to fix it.

Please push to all branches.

Jeremy.
Comment 10 Derrell Lipman 2010-01-16 20:17:03 UTC
These changes look fine to me.

Derrell
Comment 11 Karolin Seeger 2010-01-18 03:29:02 UTC
Pushed to all branches.
Closing out bug report.

Thanks!