Seg fault fix

[xiangyan99]

No description provided.

[xiangyan99]

/azp run python - identity - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[chlowell]

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.

[xiangyan99]

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.

[chlowell]

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

[xiangyan99]

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

[xiangyan99]

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

[chlowell]

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.

[xiangyan99]

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

[chlowell]

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

[xiangyan99]

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

[chlowell]

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.

[xiangyan99]

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

[chlowell]

Is setattr necessary here?

[xiangyan99]

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

[chlowell]

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>)

[xiangyan99]

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

[chlowell]

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.

[xiangyan99]

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

[chlowell]

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>)