-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Seg fault fix #11933
Seg fault fix #11933
Changes from 14 commits
3fbc443
d12edff
057808c
dcb07e4
4dd9761
8de38bf
7f130b3
ab64858
3dbabd0
f6572d8
c9446d8
e9ed70e
37043c9
d602afc
40c2824
06b15a1
adcdbed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -33,7 +41,7 @@ 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] | ||
_libsecret.secret_password_free.argtypes = [ct.c_char_p] | ||
except OSError: | ||
_libsecret = None | ||
|
||
|
@@ -58,22 +66,17 @@ 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 | ||
) | ||
attributes = [_SECRET_SCHEMA_ATTRIBUTE(_c_str("service"), 0), _SECRET_SCHEMA_ATTRIBUTE(_c_str("account"), 0)] | ||
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")) | ||
setattr(schema, "flags", 2) | ||
setattr(schema, "attributes", pattributes) | ||
p_str = _libsecret.secret_password_lookup_sync( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"), | ||
|
@@ -82,13 +85,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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,14 +105,9 @@ 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(): | ||
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) | ||
|
||
with pytest.raises(CredentialUnavailableError): | ||
credential = VSCodeCredential(_client=mock_client) | ||
token = credential.get_token("scope") | ||
def test_segfault(): | ||
from azure.identity._credentials.linux_vscode_adapter import _get_refresh_token | ||
_get_refresh_token("test", "test") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There's a larger problem with trying to catch this segfault with a test case though: when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. libsecret-1.so.0 is installed in our test environments. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
|
||
@pytest.mark.skipif(not sys.platform.startswith("darwin"), reason="This test only runs on MacOS") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
# ------------------------------------ | ||
# Copyright (c) Microsoft Corporation. | ||
# Licensed under the MIT License. | ||
# ------------------------------------ | ||
|
||
from azure.identity import VSCodeCredential | ||
|
||
def test_live(): | ||
credential = VSCodeCredential() | ||
str=credential.get_token('https://vault.azure.net/.default') | ||
xiangyan99 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
print(str) | ||
|
||
if __name__ == "__main__": | ||
test_live() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Testing visual studio code credential | ||
|
||
## Test matrix | ||
|
||
- Python 2.7, 3.5.3, 3.8 | ||
- Windows, Ubuntu 18.04, Redhat Enterprise Linux 8.1, Debian 10, Mac OS | ||
|
||
## Test steps | ||
|
||
- Install Visual Studio Code from https://code.visualstudio.com/ | ||
|
||
- Launch Visual Studio Code and go to Extension tab. | ||
|
||
- Search and install Azure Storage extension | ||
|
||
- Go to Azure tab and log in with your credential | ||
|
||
- Open a terminal, install azure-core & azure-identity | ||
xiangyan99 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- Run run-test.py | ||
|
||
Expect: a valid access token is printed out. | ||
xiangyan99 marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
setattr
necessary here?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?Also, why zero the schema's memory? You assign all its fields.
There was a problem hiding this comment.
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