[jcifs] NullPointerException in SmbFile.resolveDfs()

[Michael B Allen]

On Wed, Nov 14, 2012 at 2:09 PM, Brett Johnson
<brett.michael.johnson at gmail.com> wrote:
> Mike,
>
> We are using jCIFS v1.3.17.
>
> I searched for "jcifs tconHostName NullPointerException"
> and did not find it was fixed.  Several people had encountered
> the problem.  Gabor Herr had a proposed patch that seemed to
> work for him and another person, but you did not like it, and
> left it as a TODO to fix it right.
>
> Most users complained of encountering the NPE during listFiles.
> We encountered it when closing an InputStream.  Our application
> work-around was to catch the exception and ignore it, since we
> had consumed the input stream and didn't care.

Hi Brett,

Ok, I have added your message as a reference to the TODO list item
from Gabor Herr (although it's not crystal clear to me that it's the
same problem).

> Coincidentally, the google search also returned this page:
> http://blog.gmane.org/gmane.network.samba.java/month=20120301
>
> We have also encountered this deadlock (with a different customer).
> Although our app is multi-threaded, we were *not* using the same
> SmbFile in different threads.  The deadlock occurred when accessing
> two different files.  I fixed it in a manner nearly identical to
> that proposed by Xavier Roche, and the customer reports no more
> deadlock after a week of testing.

If you find what you think is a deadlock, it is customary to post a thread dump.

If someone posts "this is what I did and it fixes it so you should
apply this patch" it will almost certainly not be applied. It will be
treated simply as a data point to be considered in a real diagnosis. I
do not blindly apply patches from users (no one should) and it is my
experience that patches from users are almost always incorrect in some
way. It might look like it fixes their particular problem and it may
even be correct in this case but it doesn't prove that it is right. If
I blindly applied patches from user's, JCIFS would slowly degenerate
into a game of Whac-A-Mole like every other open source project out
there.

Note that Xavier's deadlock post is already on the TODO. If you
provide a thread dump I will add your post (or a reference to yours if
it's the same deadlock).

Mike

> On Nov 14, 2012, at 7:08 AM, Michael B Allen <ioplex at gmail.com> wrote:
>
>> Hi Brett,
>>
>> I think this has been fixed long ago (Google for "jcifs tconHostName
>> NullPointerException").
>>
>> What version of JCIFS are you using?
>>
>> Mike
>>
>> On Mon, Oct 15, 2012 at 6:09 PM, Brett Johnson
>> <brett.michael.johnson at gmail.com> wrote:
>>> One of our customers have encountered a NullPointerException
>>> thrown from deep within jCIFS while closing an input stream:
>>>
>>> From a customer log:
>>> Exception in thread "..." java.lang.NullPointerException
>>> at jcifs.smb.SmbFile.resolveDfs(SmbFile.java:668)
>>> at jcifs.smb.SmbFile.send(SmbFile.java:770)
>>> at jcifs.smb.SmbFile.close(SmbFile.java:1018)
>>> at jcifs.smb.SmbFile.close(SmbFile.java:1024)
>>> at jcifs.smb.SmbFile.close(SmbFile.java:1028)
>>> at jcifs.smb.SmbFileInputStream.close(SmbFileInputStream.java:104)
>>> at java.io.BufferedInputStream.close(Unknown Source)
>>> at com.google.enterprise.connector.filesystem.SmbInputStream.close(SmbInputStream.java:64)
>>> at com.google.enterprise.connector.util.BasicChecksumGenerator.getDigest(BasicChecksumGenerator.java:117)
>>> at com.google.enterprise.connector.util.BasicChecksumGenerator.getChecksum(BasicChecksumGenerator.java:131)
>>> at com.google.enterprise.connector.filesystem.FileInfoCache.getChecksum(FileInfoCache.java:59)
>>> at com.google.enterprise.connector.filesystem.FileDocumentSnapshot$GetUpdateSupport.getUpdate(FileDocumentSnapshot.java:384)
>>> at com.google.enterprise.connector.filesystem.FileDocumentSnapshot.getUpdate(FileDocumentSnapshot.java:141)
>>> at com.google.enterprise.connector.filesystem.FileDocumentSnapshot.getUpdate(FileDocumentSnapshot.java:35)
>>>
>>> Looking more closely at SmbFile, the NPE is thrown from this statement:
>>>   DfsReferral dr = dfs.resolve(
>>>       tree.session.transport.tconHostName,
>>>       tree.share,
>>>       unc,
>>>       auth);
>>>
>>> I know dfs itself is not null, since it is statically initialized.
>>> So the suspects are tree, tree.session, tree.session.transport.
>>> Immediately before this statement is a call to connect0.  Examining
>>> that code, tree starts out null but I don't think tree can be null after
>>> a successful call to connect().
>>>
>>> So that leaves tree.session and tree.session.transport.  Since
>>> session is a constructor arg for SmbTree, that probably is not
>>> null.  That leaves only SmbSession.transport.  When I read SmbSession,
>>> transport is initialized lazily and is always fetched via the
>>> SmbSession.transport() method.  I think that is the problem here.
>>> I think JCIFS could be patched as follows (note transport() method call):
>>>
>>>        DfsReferral dr = dfs.resolve(
>>>                    tree.session.transport().tconHostName,
>>>                    tree.share,
>>>                    unc,
>>>                    auth);
>>>
>>> However, even after this, tconHostName is very likely to be null in the
>>> current failure case, since it is populated by a call to transport.doConnect().
>>> If domain is null, dfs.resolve() will throw NPE when it dereferences
>>> domain (which it does several times).  At this point, I don't know
>>> enough about DFS and JCIFS to know how to fix the bug, so I modified
>>> com.google.enterprise.connector.filesystem.SmbInputStream.close() to
>>> catch NullPointerException and rethrow as IOException.
>>>
>>> --
>>> Brett M. Johnson
>>>
>>>
>>>
>>>
>>
>>
>>
>> --
>> Michael B Allen
>> Java Active Directory Integration
>> http://www.ioplex.com/
>



-- 
Michael B Allen
Java Active Directory Integration
http://www.ioplex.com/