[SCM] Samba Shared Repository - branch master updated - release-4-0-0alpha8-154-g826ee30

[simo]

On Thu, 2009-07-02 at 17:28 +0200, Stefan (metze) Metzmacher wrote:
> simo schrieb:
> > On Thu, 2009-07-02 at 22:58 +1000, tridge at samba.org wrote:
> >> Hi Simo,
> >>
> >>  > You can't have the 2 libraries installed at the same time.
> >>
> >> huh?
> > 
> > I meant, you can't do it without risks, of course you can physically
> > drop in a file.
> > 
> >>  > If, through dependencies, both get loaded in the same process space, and
> >>  > both use the same symbol names, you are doomed.
> >>
> >> err .. the loader is supposed to sort that out and use one. Metze has
> >> explained that it can screw up (though I don't fully understand how),
> >> and has proposed a patch that catches it if it happens. I certainly
> >> don't mind putting in that patch as a extra precaution.
> > 
> > This is how it can happen:
> > App A links talloc.so.1 and libsmbclient
> > New libsmbclient links talloc.so.2
> > 
> > At load time glibc loads talloc.so.1 into app A, then it loads
> > libsmbclient, which loads talloc.so.2
> > Now, most symbols are resolved by talloc.so.1 so glibc does not replace
> > them. A few new symbols though are not available so it loads them.
> > Simple and devastating.
> > 
> >>  > Can I ask you a question in reverse ?
> >>  > Why is it so imperative for you to break the ABI ?
> >>  > Why can't we fix the bug without breaking it ?
> >>  > I didn't see anything in this bug that couldn't be fixed and yet
> >>  > maintain the original ABI intact.
> >>
> >> The ABI change is needed to give us sane logging so we can find the
> >> bugs we have that have crept in over the years because we had an
> >> ambiguous API. That logging has found about 10 bugs so far (Andrew
> >> used it to find another one today, when running a RPC test against
> >> w2k3).
> > 
> > I understand why you did the changes, nobody claims they are inherently
> > wrong.
> > 
> >> If you can propose a way to do this with no ABI change then please
> >> propose a patch (and please don't just re-send Sam's patch, I'll
> >> address that in a separate email).
> > 
> > Attached a possible patch.
> > NOTE: I am not proposing to push this one, it compiles and should work,
> > but I want to make a few tests on a system using this new talloc with an
> > old samba to confirm it really works.
> > 
> > I had 2 choices here, one was to simply always call the new functions,
> > but this would break applications that called talloc_free/talloc_steal
> > against a context that had references. The other was to call the
> > original functions (renamed to _internal) and live with the bugs in
> > older apps.
> > 
> > I took the second route because it does not change anything for existing
> > apps, and, as soon as they are recompiled it will be fixed
> > automatically, looks like a sane tradeoff to me, but any comment is very
> > welcome as long as the discussion is polite.
> 
> +1
> 
> That patch is exactly what I was proposing (I just was to lazy and only
> made a prototype instead of a full working patch).

:)

> I think we can make _talloc_steal_internal static...

Uhmm, yes, we really should, thanks for spotting this, no need to expose
more symbols than we really need to.

> And we need to allow samba to build against 1.4.0...

Yes, I didn't change other files as I was just proposing the patch for
review not for an actual commit.

> And we should wrap the fprintf statements into #if 0 or #ifdef DEVELOPER.

Yes, +1 to this, we don't want a library to mess with fd=2 during normal
usage, you never know what you are really going to write to.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>