Is there a potential NULL-pointer de-reference in sourc3/modules/vfs_glusterfs.c?

[Ira Cooper]

Volker Lendecke <Volker.Lendecke at SerNet.DE> writes:

> On Wed, Sep 09, 2015 at 01:21:41PM -0700, Richard Sharpe wrote:
>> Hi folks,
>> 
>> In vfs_gluster_open (current master) we see:
>> 
>>         p_tmp = (glfs_fd_t **)VFS_ADD_FSP_EXTENSION(handle, fsp,
>>                                                           glfs_fd_t *, NULL);
>>         *p_tmp = glfd;
>> 
>> 
>> Shouldn't that last line be:
>> 
>>          if (p_tmp) *t_tmp = glfd
>> 
>> and maybe some other error checking?
>
> Yes, you are right. Although if that call fails it might be reasonable
> to assume that there's nothing better to do than crash :-)

As the module is currently.  Though, there's very few good answers to
the problem.  I'd argue ENOMEM is probably right... but we're probably
ill-equipped to handle it.

Honestly, I'm not sure if we should "abort" here or not.  (I can see
returning a -1 fd.)  After that, we can check for the -1 fd and just
return failure.  I'll throw a patch together.

> The other thing: gluster_open always returns the same leet fd. I'm not
> sure how other parts of Samba like this. In previous versions we had the
> deferred close queue implemented higher up. This would fail miserably
> with all fd's being the same. I would not count on Samba to never index
> anything based on the fd.

If you want to index, use the fsp itself, that is safe.

Really fd should be a "vfs dependent value" and a "void *" IMHO.  (You
can cast an int into it, if that's your thing in life.)

Anything that actually tries to reach in and use the fd value is being
intentionally fed garbage here to cause problems that are very
explicit.  We'd rather things fail fast, and hard.

To be clear: vfs_glusterfs has NEVER returned a value that was safe.  We
merely made it silly so people would realize it.

Thanks,

-Ira