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.

Sign up for GitHub

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

Seg fault fix #11933

Merged
merged 17 commits into from
Jun 17, 2020
Merged

Seg fault fix #11933

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3fbc443
update
xiangyan99 Jun 9, 2020
d12edff
update
xiangyan99 Jun 9, 2020
057808c
update
xiangyan99 Jun 9, 2020
dcb07e4
update
xiangyan99 Jun 9, 2020
4dd9761
update
xiangyan99 Jun 9, 2020
8de38bf
udpate
xiangyan99 Jun 9, 2020
7f130b3
update
xiangyan99 Jun 9, 2020
ab64858
update
xiangyan99 Jun 10, 2020
3dbabd0
secret_password_free
xiangyan99 Jun 10, 2020
f6572d8
update
xiangyan99 Jun 10, 2020
c9446d8
update
xiangyan99 Jun 11, 2020
e9ed70e
add manual test script for vscode credential
xiangyan99 Jun 16, 2020
37043c9
add license
xiangyan99 Jun 16, 2020
d602afc
update
xiangyan99 Jun 16, 2020
40c2824
update
xiangyan99 Jun 17, 2020
06b15a1
updates
xiangyan99 Jun 17, 2020
adcdbed
update
xiangyan99 Jun 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 38 additions & 27 deletions sdk/identity/azure-identity/azure/identity/_credentials/linux_vscode_adapter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,31 @@
import os
import json
import ctypes as ct
from .._constants import VSCODE_CREDENTIALS_SECTION


def _c_str(string):
return ct.c_char_p(string.encode("utf-8"))


class _SECRET_SCHEMA_ATTRIBUTE(ct.Structure):
_fields_ = [
("name", ct.c_char_p),
("type", ct.c_uint),
]


class _SECRET_SCHEMA(ct.Structure):
_fields_ = [
("name", ct.c_char_p),
("flags", ct.c_uint),
("attributes", _SECRET_SCHEMA_ATTRIBUTE * 2),
]
_PSECRET_SCHEMA = ct.POINTER(_SECRET_SCHEMA)


try:
_libsecret = ct.cdll.LoadLibrary("libsecret-1.so.0")
_libsecret.secret_schema_new.argtypes = [
ct.c_char_p,
ct.c_uint,
ct.c_char_p,
ct.c_uint,
ct.c_char_p,
ct.c_uint,
ct.c_void_p,
]
_libsecret.secret_password_lookup_sync.argtypes = [
ct.c_void_p,
ct.c_void_p,
Expand All @@ -33,7 +41,6 @@ def _c_str(string):
ct.c_void_p,
]
_libsecret.secret_password_lookup_sync.restype = ct.c_char_p
_libsecret.secret_schema_unref.argtypes = [ct.c_void_p]
except OSError:
_libsecret = None

Expand All @@ -58,22 +65,23 @@ def _get_refresh_token(service_name, account_name):
if not _libsecret:
return None

# _libsecret.secret_password_lookup_sync raises segment fault on Python 2.7
# temporarily disable it on 2.7
import sys

if sys.version_info[0] < 3:
raise NotImplementedError("Not supported on Python 2.7")

if sys.version_info >= (3, 8):
raise NotImplementedError("Not supported")

err = ct.c_int()
schema = _libsecret.secret_schema_new(
_c_str("org.freedesktop.Secret.Generic"), 2, _c_str("service"), 0, _c_str("account"), 0, None
)
attribute1 = _SECRET_SCHEMA_ATTRIBUTE()
setattr(attribute1, "name", _c_str("service"))
setattr(attribute1, "type", 0)
attribute2 = _SECRET_SCHEMA_ATTRIBUTE()
setattr(attribute2, "name", _c_str("account"))
setattr(attribute2, "type", 0)
attributes = [attribute1, attribute2]
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved
pattributes = (_SECRET_SCHEMA_ATTRIBUTE * 2)(*attributes)
schema = _SECRET_SCHEMA()
pschema = _PSECRET_SCHEMA(schema)
ct.memset(pschema, 0, ct.sizeof(schema))
setattr(schema, "name", _c_str("org.freedesktop.Secret.Generic"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setattr necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from what I know, how to set value for c-like structures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An instance already has the attributes though:

>>> schema = _SECRET_SCHEMA()
>>> schema.name, schema.flags, schema.attributes
(None, 0, <__main__._SECRET_SC...x04F150B8>)

>>> schema = _SECRET_SCHEMA(name=_c_str("org.freedesktop.Secret.Generic"), flags=2, attributes=pattributes)
>>> schema.name, schema.flags, schema.attributes
(b'org.freedesktop.Secret.Generic', 2, <__main__._SECRET_SC...x04F15418>)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is ct.memset(pschema, 0, ct.sizeof(schema))
by calling
schema = _SECRET_SCHEMA(name=_c_str("org.freedesktop.Secret.Generic"), flags=2, attributes=pattributes) only,
it is not guaranteed the rest of the memory is set to 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how that requires setattr. Wouldn't this still work?

schema = _SECRET_SCHEMA()
pschema = _PSECRET_SCHEMA(schema)
ct.memset(pschema, 0, ct.sizeof(schema))
schema.name = _c_str("org.freedesktop.Secret.Generic")

Also, why zero the schema's memory? You assign all its fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes is an array which can be more than 2. In our case, we make 2 in the definition but no harm to set it. We only call it once so the cost is negligible

setattr(schema, "flags", 2)
setattr(schema, "attributes", pattributes)
p_str = _libsecret.secret_password_lookup_sync(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return is just a pointer, but the documentation says it should be freed with secret_password_free. I take that to imply secret_password_lookup_sync may allocate memory internally which ctypes wouldn't free.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the gnome implementation. If we call secret_passwor_free, it complains invalid pointer. Given .net does not call it either (Azure/azure-sdk-for-net#10979) and it only calls once, I will keep it.

schema,
pschema,
None,
ct.byref(err),
_c_str("service"),
Expand All @@ -82,13 +90,16 @@ def _get_refresh_token(service_name, account_name):
_c_str(account_name),
None,
)
_libsecret.secret_schema_unref(schema)
if err.value == 0:
return p_str.decode("utf-8")

return None


def get_credentials():
# Disable linux support for further investigation
raise NotImplementedError("Not supported")
try:
environment_name = _get_user_settings()
credentials = _get_refresh_token(VSCODE_CREDENTIALS_SECTION, environment_name)
return credentials
except Exception: # pylint: disable=broad-except
return None
10 changes: 3 additions & 7 deletions sdk/identity/azure-identity/azure/identity/_credentials/vscode_credential.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,9 @@ def get_token(self, *scopes, **kwargs):
return token

if not self._refresh_token:
try:
self._refresh_token = get_credentials()
if not self._refresh_token:
raise CredentialUnavailableError(message="No Azure user is logged in to Visual Studio Code.")
except NotImplementedError: # pylint:disable=try-except-raise
raise CredentialUnavailableError(message="Not supported")

self._refresh_token = get_credentials()
if not self._refresh_token:
raise CredentialUnavailableError(message="No Azure user is logged in to Visual Studio Code.")

token = self._client.obtain_token_by_refresh_token(scopes, self._refresh_token, **kwargs)
return token
9 changes: 3 additions & 6 deletions sdk/identity/azure-identity/azure/identity/aio/_credentials/vscode_credential.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,9 @@ async def get_token(self, *scopes, **kwargs):
return token

if not self._refresh_token:
try:
self._refresh_token = get_credentials()
if not self._refresh_token:
raise CredentialUnavailableError(message="No Azure user is logged in to Visual Studio Code.")
except NotImplementedError: # pylint:disable=try-except-raise
raise CredentialUnavailableError(message="Not supported")
self._refresh_token = get_credentials()
if not self._refresh_token:
raise CredentialUnavailableError(message="No Azure user is logged in to Visual Studio Code.")

token = await self._client.obtain_token_by_refresh_token(scopes, self._refresh_token, **kwargs)
return token
5 changes: 4 additions & 1 deletion sdk/identity/azure-identity/tests/test_vscode_credential.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,14 @@ def test_no_obtain_token_if_cached():


@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="This test only runs on Linux")
def test_distro():
def test_segfault():
from azure.identity._credentials.linux_vscode_adapter import _get_refresh_token
mock_client = mock.Mock(spec=object)
mock_client.obtain_token_by_refresh_token = mock.Mock(return_value=None)
mock_client.get_cached_access_token = mock.Mock(return_value=None)

_get_refresh_token("test", "test")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an odd addition to, or repurposing of, this test. Do you mean to keep it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rename the test to test_segfault and want to use it as the seg fault regression test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I want to make sure calling into _get_refresh_token does not cause seg fault.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that's the intent of this line. What I mean is, the existing code uses a mock client to test whether the credential raises CredentialUnavailableError appropriately (I think the code is incorrect though). That's unrelated to whether the native interop causes a segfault, so testing both behaviors at once seems odd to me. But after taking a closer look at the existing code, I see it already calls _get_refresh_token, so an explicit call here is redundant anyway.

There's a larger problem with trying to catch this segfault with a test case though: when libsecret-1.so.0 isn't available _get_refresh_token returns None without invoking any native code. When the test passes we can't say whether the code we meant to test works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libsecret-1.so.0 is installed in our test environments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know that only because we've seen the segfault in CI runs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We saw seg fault in CI which meant libsecret-1.so.0 was installed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it was installed on the agents that segfaulted. That doesn't mean it's installed on all agents, or always installed. If we don't know it's installed, we don't learn anything when the test passes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. So I think this one + some manual testing should have a good coverage.


with pytest.raises(CredentialUnavailableError):
credential = VSCodeCredential(_client=mock_client)
token = credential.get_token("scope")
Expand Down
14 changes: 0 additions & 14 deletions sdk/identity/azure-identity/tests/test_vscode_credential_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,17 +106,3 @@ async def test_no_obtain_token_if_cached():
credential = VSCodeCredential(_client=mock_client)
token = await credential.get_token("scope")
assert token_by_refresh_token.call_count == 0


@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="This test only runs on Linux")
@pytest.mark.asyncio
async def test_distro():

mock_client = mock.Mock(spec=object)
token_by_refresh_token = mock.Mock(return_value=None)
mock_client.obtain_token_by_refresh_token = wrap_in_future(token_by_refresh_token)
mock_client.get_cached_access_token = mock.Mock(return_value=None)

with pytest.raises(CredentialUnavailableError):
credential = VSCodeCredential(_client=mock_client)
token = await credential.get_token("scope")