Bug 7045 - Bad (non memory copying) interfaces in smbc_setXXXX calls.
Summary: Bad (non memory copying) interfaces in smbc_setXXXX calls.
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: unspecified
Hardware: Other All
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
Depends on:
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+

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)
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:


  /* 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.

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 :



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.


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?

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.

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

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