-
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 all 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 |
---|---|---|
|
@@ -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() | ||
token=credential.get_token('https://vault.azure.net/.default') | ||
print(token) | ||
|
||
if __name__ == "__main__": | ||
test_live() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# 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 latest azure-identity by running | ||
```python | ||
pip install azure-identity -i https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-python%40Local/pypi/simple/ | ||
``` | ||
|
||
- Run run-test.py | ||
|
||
Expect: an access token is printed out. |
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 return is just a pointer, but the documentation says it should be freed with
secret_password_free
. I take that to implysecret_password_lookup_sync
may allocate memory internally which ctypes wouldn't free.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.
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.