[Gluster-devel] problem with recent change to glfs_realpath

[Michael Adam]

On 2016-10-21 at 10:22 +0200, Niels de Vos wrote:
> On Fri, Oct 21, 2016 at 01:15:50AM +0200, Michael Adam wrote:
> > Hi all,
> > 
> > Anoop has brought to my attention that
> > recently glfs_realpath was changed in an incompatible way:
> 
> Interesting, Rajesh and I had an email discussion about that yesterday
> too... Unfortunately this (or the Samba) list was not on CC :-/

Yeah, Anoop brought it up, but Anoop, Rajesh and I discussed
the problem and the solution. Since I saw now mail on the list
I raised it, but it's got that common background. :-)

> > Previously, glfs_realpath returned an allocated string
> > that the caller would have to free with 'free'. Now  after
> > the change, free segfaults on the returned string, and
> > the caller needs to call glfs_free.
> > 
> > That change makes no sense, imho, because the result from
> > a realpath implementation may be used by the application
> > using libgfapi, outside of the scope of the actual libgfapi
> > using code. E.g. in samba, the gfapi calls are hidden behind
> > the vfs api in the gluster backend. But the realpath result
> > is used outside the vfs module. I think this should be quite
> > normal a use case, and hence glfs_realpath should behave
> > as one would expect from a realpath implementation
> > (and as described in the realpath(3) manpage): return a string
> > that needs to be freed with 'free'...
> 
> With libgfapi and other applications we can not guarantee that the
> malloc() that libgfapi uses matches the free() from the application. It
> is possible for applications to link with alternative implementations
> (libjemalloc for example). All structures allocated and returned by
> libgfapi should be free'd by libgfapi as well. We have seen problems
> like this with NFS-Ganesha before, and that triggered us to introduce
> glfs_free().

Hm. Ok, I see what you're saying here, but doesn't that apply
to many other libraries as well? libc functions allocate
memory for you frequently. (Eg libc's realpath.) So no
application should ever use these? It kind of questions the
fundamentals of many such libs.

> > Now for samba, after thorough discussion with Anoop and Rajesh,
> > we have proposed a fix/workaround by using the variant of
> > glfs_realpath that hands in a pre-allocated result string.
> > This renders us independent of the allocation method used by
> > glfs_realpath. But we wanted to point this out to the list, since
> > it is a potential problem for other users, too.
> 
> That is the correct approach. I am very sorry that I missed to check
> samba/vfs_gluster for glfs_realpath() usage, none of the other projects
> that I am aware of use this function.

No damage done, just some amount of initial confusion.
It is just in master yet. By early (manual)  upstream testing
Anoop found it. We should aim to include samba in some upstream
automated sanity test at least.

> I was (and still am!) planning to write a blog post and email about this
> general glfs_free() addition/change.

Sure, that might be a good idea!

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20161021/6ee51856/attachment.sig>