New issue
Advanced search Search tips

Issue 874964 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 28
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Remove use of smbc_getdents

Project Member Reported by zentaro@chromium.org, Aug 16

Issue description

smbc_getdents supplies the result buffer as a concatenation of variable length structs with flexible inline strings. The code was casting a pointer to smbc_dirent and advancing it along the buffer.

On devices with alignment requirements eg. aarch64 devices like Kevin this causes alignment faults.

The fix is to use smbc_readdir instead which only reads a single entry at a time. This is only used in the case of share enumeration, since the regular read directory case uses smbc_readdirplus which also returns that metadata that would normally require a call to smbc_stat() in a single call.

However smbc_readdirplus can't be used during share enumeration, because as we try to get metadata for the shares authentication callbacks will be triggered and often contain hidden/system special shares like IPC$ or printer shares. When these can't be authenticated to smbc_readdirplus stops reading the directory and doesn't even return the names of the shares which are required for discovery.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/63191213b4e126385b8b25921d357650215f54b4

commit 63191213b4e126385b8b25921d357650215f54b4
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:06 2018

smbprovider: Add function pointers for smbc_readdir

- To avoid dealing with the unaligned buffer that is returned
  from smbc_getdents, future CL will change to using smbc_readdir
  to read a single entry at a time.

BUG= chromium:874964 
TEST=emerges

Change-Id: I9c42fc845852218bf8f5664b0118130e3d4ee3f0
Reviewed-on: https://chromium-review.googlesource.com/1178694
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: jimmy gong <jimmyxgong@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/63191213b4e126385b8b25921d357650215f54b4/smbprovider/samba_interface_impl.h
[modify] https://crrev.com/63191213b4e126385b8b25921d357650215f54b4/smbprovider/samba_interface_impl.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 20

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/b2f3ee18eaa034315cd48dd267781929902c9427

commit b2f3ee18eaa034315cd48dd267781929902c9427
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:07 2018

smbprovider: Add implementation of smbc_readdir

BUG= chromium:874964 
TEST=emerges

Change-Id: I9421674b17fef195801d44982cde0063e65cc28e
Reviewed-on: https://chromium-review.googlesource.com/1178696
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/b2f3ee18eaa034315cd48dd267781929902c9427/smbprovider/samba_interface_impl.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/f0681d1c565fd0a3a46a1076e06affa49083ebcf

commit f0681d1c565fd0a3a46a1076e06affa49083ebcf
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:08 2018

smbprovider: Add fake impl of GetDirectoryEntry and tests

- Adds fake implementation that reads one smbc_dirent at a time
- Add tests

BUG= chromium:874964 
TEST=emerges

Change-Id: Iace9845e905947059c9d2cf445c675335232a313
Reviewed-on: https://chromium-review.googlesource.com/1179807
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/f0681d1c565fd0a3a46a1076e06affa49083ebcf/smbprovider/fake_samba_interface.h
[modify] https://crrev.com/f0681d1c565fd0a3a46a1076e06affa49083ebcf/smbprovider/fake_samba_interface.cc
[modify] https://crrev.com/f0681d1c565fd0a3a46a1076e06affa49083ebcf/smbprovider/fake_samba_test.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/816f0c03dc51279434f235608061f9fd5e9510f6

commit 816f0c03dc51279434f235608061f9fd5e9510f6
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:08 2018

smbprovider: Update the iterator to not use smbc_getdents

- Switches the "no metadata" code path to use smbc_readdir
  to read only one entry at a time.
- Remove a white box test that was specific to the old
  implementation.

BUG= chromium:874964 
TEST=unittests

Change-Id: Ic2871f1cfdc3d3733e4acb0bd2a8944133dcf23b
Reviewed-on: https://chromium-review.googlesource.com/1179808
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/816f0c03dc51279434f235608061f9fd5e9510f6/smbprovider/iterator/directory_iterator_test.cc
[modify] https://crrev.com/816f0c03dc51279434f235608061f9fd5e9510f6/smbprovider/iterator/directory_iterator.cc
[modify] https://crrev.com/816f0c03dc51279434f235608061f9fd5e9510f6/smbprovider/iterator/directory_iterator.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1adf32c3f981753cc35cf697431091d2e40e0cfc

commit 1adf32c3f981753cc35cf697431091d2e40e0cfc
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:09 2018

smbprovider: Move WriteEntry to test only file

- WriteEntry writes an smbc_dirent with a trailing flexible string
- As part of removing uses of smbc_getdents this is only used by
  test code now to generate a single struct on an aligned buffer.

BUG= chromium:874964 
TEST=unittests

Change-Id: If68c7d1f65e00ea59c6247ebffae55b8aed93e68
Reviewed-on: https://chromium-review.googlesource.com/1179816
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/1adf32c3f981753cc35cf697431091d2e40e0cfc/smbprovider/smbprovider_helper.cc
[modify] https://crrev.com/1adf32c3f981753cc35cf697431091d2e40e0cfc/smbprovider/smbprovider_helper.h
[modify] https://crrev.com/1adf32c3f981753cc35cf697431091d2e40e0cfc/smbprovider/fake_samba_interface.cc
[modify] https://crrev.com/1adf32c3f981753cc35cf697431091d2e40e0cfc/smbprovider/smbprovider_helper_test.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1fbecd1226dbd2116281a0e249b8a5c6051b13eb

commit 1fbecd1226dbd2116281a0e249b8a5c6051b13eb
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:09 2018

smbprovider: Remove unused smbc_getdents code from iterator

- Remove code that used to support smbc_getdents from the
  iterator as part of removing all uses of this API.

BUG= chromium:874964 
TEST=unittests

Change-Id: I4b476c59bb253feb462e26b954ebf54e8452bc62
Reviewed-on: https://chromium-review.googlesource.com/1179817
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/1fbecd1226dbd2116281a0e249b8a5c6051b13eb/smbprovider/iterator/directory_iterator.cc
[modify] https://crrev.com/1fbecd1226dbd2116281a0e249b8a5c6051b13eb/smbprovider/iterator/directory_iterator.h

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/7c573af98cd0f4a6774325164e3d71579b6730aa

commit 7c573af98cd0f4a6774325164e3d71579b6730aa
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:10 2018

smbprovider: Remove all code related to smbc_getdents

- Since the buffer returned contains unaligned structs embedded
  within a buffer we are no longer using this API.
- All uses were either already replaced with smbc_readdirplus or
  smbc_readdir
- Removes helper functions only used to support this API

BUG= chromium:874964 
TEST=unittests

Change-Id: I98dd73eb8260b39e77afca6b18c6b94572635ec7
Reviewed-on: https://chromium-review.googlesource.com/1179818
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/fake_samba_proxy.cc
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/fake_samba_proxy.h
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/smbprovider_helper.h
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/fake_samba_interface.cc
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/samba_interface.h
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/samba_interface_impl.cc
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/fake_samba_interface.h
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/smbprovider_helper.cc
[modify] https://crrev.com/7c573af98cd0f4a6774325164e3d71579b6730aa/smbprovider/samba_interface_impl.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/7e4071906842f94675ca585faeec637e4d88b4a8

commit 7e4071906842f94675ca585faeec637e4d88b4a8
Author: Zentaro Kavanagh <zentaro@chromium.org>
Date: Mon Aug 20 19:20:10 2018

smbprovider: Align authentication callback

- Unaligned pointer causes alignment fault on aarch64 devices

BUG= chromium:874964 
TEST=unittests and ran on Kevin no crash anymore

Change-Id: I103095860d505b30a4695bf75d476491e2b8c533
Reviewed-on: https://chromium-review.googlesource.com/1179965
Commit-Ready: Zentaro Kavanagh <zentaro@chromium.org>
Tested-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>

[modify] https://crrev.com/7e4071906842f94675ca585faeec637e4d88b4a8/smbprovider/samba_interface_impl.cc

Status: Fixed (was: Started)

Sign in to add a comment