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.

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

check_duplicate: check name member before using it #70

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@sumit-bose
Copy link
Contributor

commented Nov 4, 2016

The second patch resolves https://fedorahosted.org/sssd/ticket/3231 and adds a
test for the issue.

The first patches fixes a potential memory leak which so far was only relevant
in the tests.

@celestian celestian self-assigned this Nov 7, 2016

* - EOK success
* - ENOMEM memory allocation failed
* - ERR_DUP_EXTRA_ATTR sysdb attribute is already used
*/

This comment has been minimized.

Copy link
@celestian

celestian Nov 7, 2016

Collaborator

I agree that the function API comment is useful. In my opinion -- since this function isn't static -- isn't better place for it in sdap.h file?

This comment has been minimized.

Copy link
@sumit-bose

sumit-bose Nov 7, 2016

Author Contributor

You are right, sdap.h is the more suitable place. New version pushed,

@celestian

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2016

The patch set LGTM.
I am waiting for CI and I would like to test it manually.

sumit-bose added some commits Nov 4, 2016

sdap_extend_map: make sure memory can be freed
If there is an error after calling talloc_realloc() the caller cannot
free the memory properly because neither src_map nor _map were pointing
to a valid memory location. With this patch _map will always point to
the current valid location so that it can always be used with
talloc_free().

@sumit-bose sumit-bose force-pushed the sumit-bose:fix_check_duplicate branch from f83e18d to bfead81 Nov 7, 2016

@celestian

This comment has been minimized.

Copy link
Collaborator

commented Nov 8, 2016

I tested manually with IPA provider. It works and I was informed about attribute colision:

[sdap_extend_map] (0x0010): Attribute entryUSN (abc in LDAP) is already used by SSSD, please choose a different cache name

CI tests passed:
http://sssd-ci.duckdns.org/logs/job/56/33/summary.html

=> ACK

@celestian celestian added the Accepted label Nov 8, 2016

@lslebodn lslebodn added Changes requested and removed Accepted labels Nov 8, 2016

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2016

I would appreciate integration test for the crash.
Feel free to extend test_extra_attribute_already_exists or create new one.

@sumit-bose

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2016

@lslebodn, the patch adds a new unit-test test_extra_opts_empty_name() which covers the crash. What would be the benefit of checking it in the integration tests as well?

@celestian

This comment has been minimized.

Copy link
Collaborator

commented Nov 21, 2016

@lslebodn, Lukas, are you satisfied by Sumit's explanation?

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

I'm not sure if it's even possible to write an integration test because even after the patch, sssd doesn't start, it 'just' doesn't segfault.

@jhrozek jhrozek added Accepted and removed Changes requested labels Feb 10, 2017

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2017

Can we have this one pushed by @sumit-bose and @jhrozek review?

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

Actually, let's push this PR now, there is a test so we won't regress

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

master:
    454cf0c3808a9f6a0c9f79e9796e17c58907ee6c
    08bf6b4a281ef4308119dccbba4e86cf28b505d2 
sssd-1-14:
    c14980e81253aaec2fddb4f794fb1eb39167e885
    bb4b624bfb3a08fc3b2989d0cce05afd2c3d4843 

@jhrozek jhrozek added the Pushed label Feb 22, 2017

@jhrozek jhrozek closed this Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.