Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip DFS referrals without referral_entries #149

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

MWedl
Copy link
Contributor

@MWedl MWedl commented Oct 25, 2021

Hi,
I have encountered an IndexError in class ReferralEntry when DFSReferralResponse contains no referral_entries (i.e. number_of_referrals is 0). ReferralEntry assumes that there is always at least one entry available.

In my pull request I decided to not create instances of ReferralEntry at all when number_of_referrals is 0. This is outside of the class and does not mitigate the root problem. However, the only place where ReferralEntry instances are created is in ClientConfig.cache_referral().

Another possibility would be to check if referral_entries are available in each method of ReferralEntry and return None. I think this would cause further problems since methods do no longer guarantee to return a value but None instead. This also needs adaptions in code calling these methods.

I would appreciate feedback what you think is the best way to solve this error.

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #149 (072f09c) into master (88a97a0) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   98.93%   99.09%   +0.15%     
==========================================
  Files          23       23              
  Lines        5052     5059       +7     
==========================================
+ Hits         4998     5013      +15     
+ Misses         54       46       -8     
Flag Coverage Δ
macOS 68.19% <87.50%> (+1.01%) ⬆️
py3.10 99.07% <100.00%> (+0.15%) ⬆️
py3.6 99.07% <100.00%> (+0.15%) ⬆️
py3.7 99.07% <100.00%> (+0.15%) ⬆️
py3.8 99.06% <100.00%> (+0.15%) ⬆️
py3.9 99.08% <100.00%> (+0.15%) ⬆️
ubuntu 96.77% <100.00%> (+0.16%) ⬆️
windows 99.00% <100.00%> (+0.16%) ⬆️
x64 99.09% <100.00%> (+0.15%) ⬆️
x86 99.00% <100.00%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
smbclient/_io.py 99.03% <100.00%> (+<0.01%) ⬆️
smbclient/_pool.py 95.89% <100.00%> (+4.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88a97a0...072f09c. Read the comment docs.

@jborean93
Copy link
Owner

Interesting, I’ll have to have a closer look at the DFS docs to see when this would occur. Do you have more information about the environment where this occurred for you?

@MWedl
Copy link
Contributor Author

MWedl commented Oct 25, 2021

This error occured on .DFSFolderLink which seems to be the a remainder/side-effect of the powershell cmdlet New-DfsnFolder. I just encountered this while crawling some shares and do no have more details. I hope this helps you.

@jborean93
Copy link
Owner

No worries, I'll have to try and replicate but considering it's happening in real life I don't see why this check can't occur

@jborean93
Copy link
Owner

The docs seems to at least mention this is possible

If the referral request is successful, but the NumberOfReferrals field in the referral header (as specified in section 2.2.4) is 0, the DFS server could not find suitable targets to return to the client. In this case, the client MUST fail the original I/O operation with STATUS_OBJECT_PATH_NOT_FOUND.

Seems like more handling will need to be done to check that lookup_referral returns an actual value otherwise STATUS_OBJECT_PATH_NOT_FOUND should be raised. I'll have a quick play and see if I can get something together to push to this PR.

@jborean93
Copy link
Owner

I've pushed a commit that adds the exception handling as well as some hacky tests. If you get a change it would be great if you could try it out on your environment to make sure I didn't break anything. I would expect a DFS share with no referrals to raise ObjectPathNotFound.

@MWedl
Copy link
Contributor Author

MWedl commented Oct 27, 2021

Thanks for your commit and looking up how to handle empty referrals in the documentation.

You missed one place (smbclient._io._resolve_dfs) where ObjectPathNotFound should also be thrown. I have added the check and a test case for it.

Now everything works as expected in my environment. Feel free to merge.

@jborean93
Copy link
Owner

You missed one place (smbclient._io._resolve_dfs) where ObjectPathNotFound should also be thrown. I have added the check and a test case for it.

Ah thanks for the pickup, I completely forgot about this use of DFS.

Appreciate the time and effort fixing this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants