[SCM] Samba Shared Repository - branch master updated

Jeremy Allison jra at samba.org
Fri Jan 15 18:21:48 MST 2010


On Fri, Jan 15, 2010 at 08:07:21PM -0500, Derrell Lipman wrote:
> On Fri, Jan 15, 2010 at 19:45, Jeremy Allison <jra at samba.org> wrote:
> 
>     On Thu, Jan 14, 2010 at 12:50:26PM -0600, G nther Deschner wrote:
> 
>     > +/* note the strdup for string options on smbc_set calls. 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. */
> 
>     God you're right - that's *REALLY* horrible. libsmbclient
>     should have been deep copying the strings from the start,
>     not just stealing the pointers.
> 
> 
> 
> I missed the beginning of this conversation. The message including Gunther's
> above comment didn't arrive here.

It was a git commit message.

> I'm looking at smbc_free_context for free() calls to see what this is referring
> to. I see that I free the return value of smbc_getWorkgroup(), etc, thus the
> smbc_set calls it refers to would be the like of smbc_setWorkgroup(). I'm
> pretty sure that these were previously simple assignments before I added the
> set/get interface, so it's "always" been this way, with the assumption that
> those values are malloc'ed. That not withstanding, certainly, if that was the
> assumption, it would have been nice if it had been documented it in
> libsmbclient.h.

Yes, I'm sure it's one of these things that have "always" been that
way, but IMHO we really need to fix this asap. I know it might cause
a memory leak in code that currently uses it "correctly" but I can't
find any code out there that does do this correctly (except Guenthers
test code) so I don't think this will be a problem. It's so
counter-intuitive !

> Converting this to a deep copy is perfectly reasonable, with the caveat that
> code expecting their pointer to be freed will end up with a memory leak if not
> otherwise changed.

I'm going to commit a patch to master that does this, with an associated
bug:

https://bugzilla.samba.org/show_bug.cgi?id=7045

so we can get this fixed in current shipping code.

Jeremy.


More information about the samba-technical mailing list