[PATCH] PAM: Do not leak fd after SELinux context file is written
by Jakub Hrozek
https://fedorahosted.org/sssd/ticket/1619
We don't close the fd when we write the selinux login file in the pam
responder. This results in a fd leak.
---
src/responder/pam/pamsrv_cmd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 2b20544d1baf78ade232f93a2c91378ec6a7645c..bb0d8db384cbbc8a3beed5dc5be0fe32956992a9 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -443,6 +443,7 @@ static errno_t write_selinux_login_file(const char *username, char *string)
} else {
ret = EOK;
}
+ close(fd);
fd = -1;
done:
--
1.7.12.1
11 years, 7 months
[PATCH] sss_cache: Remove fastcache even if sssd is not running.
by Michal Židek
https://fedorahosted.org/sssd/ticket/1584
First, I thought the race condition between sssd_nss and sss_cache
should be solved by some sort of file locking mechanism, but when
started working on it, the places where we needed to check for the file
being locked or free were too many and spread among monitor, nss and
sss_cache tool processes and it was not clear how the access is controlled.
So I decided to do it this way:
1. sss_cache tries to send_sighup to monitor as usual
2. if signal_sssd returns that sssd is not running, proceed with
memcache invalidation
3. As a part of memcache invalidation:
- it first opens the mc file
- then it checks if sssd is running (with pgrep)
- if sssd is running it stops the the process of invalidation
- if sssd is still not running it proceeds with the invalidation
See that if sssd starts after (or during) the pgrep check (so we will
not catch it as running, but will assume it is off) it is not a
problem, because we have file descriptor associated with file that was
present before sssd was running (we open the file before pgrep call).
sssd_nss alwas creates a new memory cache file on startup, so we will
only mark the old one as recycled (and not the new one), that we do not
care about (because it will be deleted by sssd_nss and marking that as
recycled is not a problem, I think -- the worst thing that can happen is
race between nss and cache tool while both are marking the OLD file as
recycled, but I can not figure out situation where this could be
dangerous, because there are no "temp" states that could be harmful.
Both processes are changing the value of status, but both to the same
value).
Another thing that I like about this is that we do not have to care
about communication sssd vs sssd_nss vs sss_cache but only control sssd
vs sss_cache, which is much easier to understand.
Can someone see any problems that I missed?
NOTE: The function sss_mc_set_recycled is copied from different module.
I did not want to make this function non static and put it to a header
file because it is not intended to be used directly.
I tested it and it works fine for me.
The patch is attached.
Thanks
Michal
11 years, 7 months
[PATCH] sss_cache: Multiple domains not handled properly
by Michal Židek
I found this bug while working on:
https://fedorahosted.org/sssd/ticket/1584
If no objects for deletion were found in the currently searched sysdb
database, the other sysdb databases were not searched at all.
Also the ERROR message informing about specific object not found was
changed to DEBUG message. It was really annoying seeing this message for
all domains, but it can be useful for debugging (it helped me by testing
this patch). Now only one ERROR message is printed if no matching
objects where found and all domains were searched.
Patch is attached.
Thanks
Michal
11 years, 7 months
Re: [SSSD] [Freeipa-devel] [RANT] Patchwork process
by Dmitri Pal
On 11/02/2012 07:22 AM, Petr Spacek wrote:
> On 11/02/2012 11:10 AM, Petr Viktorin wrote:
>> On 11/02/2012 10:46 AM, Martin Kosek wrote:
>>> On 11/01/2012 07:28 PM, Simo Sorce wrote:
>>>> On Thu, 2012-11-01 at 10:59 -0400, Rob Crittenden wrote:
>>>>> Rob Crittenden wrote:
>>>>>> Simo Sorce wrote:
>>>>>>> From: Simo Sorce <ssorce(a)redhat.com>
>>>>>>>
>>>>>>> We check (possibly different) data from LDAP only at (re)start.
>>>>>>> This way we always shutdown exactly the services we started even if
>>>>>>> the list
>>>>>>> changed in the meanwhile (we avoid leaving a service running
>>>>>>> even if
>>>>>>> it was
>>>>>>> removed from LDAP as the admin decided it should not be started in
>>>>>>> future).
>>>>>>>
>>>>>>> This should also fix a problematic deadlock with systemd when we
>>>>>>> try
>>>>>>> to read
>>>>>>> the list of service from LDAP at shutdown.
>>>>>>
>>>>>> This fixes things for me but ipactl start isn't working reliably and
>>>>>> I've yet to determine if it is related or not (I'm thinking not).
>>>>>>
>>>>>> What I see is that it considers pki-tomcatd to not have started.
>>>>>> What is
>>>>>> happening is the request to the getStatus URI is timing out and that
>>>>>> exception is being considered a "didn't start."
>>>>>>
>>>>>> I modified the code to treat a timeout as a 503 and it is still
>>>>>> failing
>>>>>> because the timeout is so longer that it eats up all our normal
>>>>>> timeout
>>>>>> for waiting for the service at all.
>>>>>>
>>>>>> I don't see a way to pass a timeout to the http request method, I'll
>>>>>> keep looking.
>>>>>>
>>>>>> I'm testing in F-18 btw.
>>>>>
>>>>> I can't reproduce the startup issues this morning, I still don't
>>>>> think
>>>>> they are related to this patch, so ACK.
>>>>>
>>>>> pushed all 3 to master and ipa-3-0
>>>>
>>>> You pushed an older revision for patch 2, I reverted it on both trees
>>>> and pushed the right one.
>>>>
>>>> Simo.
>>>
>>> While trying to follow this thread, I must throw my 2 conservative
>>> cents in.
>>> This thread is a very good example why processing our patches via
>>> patchwork is
>>> IMHO rather fuzzy and can cause more confusion than clarity. These
>>> are my main
>>> points:
>>>
>>> I cannot see which patches are the recent ones. Each patch is sent
>>> separately
>>> in mail body and I cannot quickly see by looking in thread which
>>> mail contains
>>> a revised patch and which is just a comment.
>>>
>>> When patches are attached directly to mail as files, I can quickly
>>> distinguish
>>> which mail has new patch set as it has "the paperclip icon". It also
>>> enabled us
>>> to attach a whole set of valid patches to one mail and thus make it
>>> easier for
>>> reviewer to pull a whole set of valid patches in one click.
>> >
>>>
>>> Patchwork style patch sending also generates a lot of mails which
>>> may confuse
>>> not only me but other contributors...
>>
>> +1
>>
>> This particular thread is extremely confusing. Each individual patch
>> generated
>> its own subthread, and then the revised patches were sent as a reply the
>> original, spawning new threads to continue the old ones. The
>> discussion is
>> broken up and impossible to navigate. You can't see where the patches
>> are, let
>> alone where to look for latest versions.
>> Thanks to Rob for showing us that even a person following the thread
>> is easily
>> confused.
>>
>> Patchwork adds a view that is incomplete and not cross-linked.
>> To make a comment on a patch, you need to do it by e-mail; finding a
>> patch in
>> Patchwork is useless for this. Same for assigning a reviewer: you
>> need to find
>> it "manually" in Patchwork even if you have the mail in front of you.
>>
>>
>> I don't really see Patchwork's advantage. For a list of "active"
>> patches, we
>> have the "Patch submitted" bit in Trac; to claim a larger review we
>> could have
>> a policy to announce this on the list. And the claim that Patchwork
>> doesn't
>> change our current practices is simply false.
>>
>
> As guys described above, Patchwork influenced our process a lot and,
> personally, I don't like these changes.
>
> +1 from me to more rational Trac usage. We can simply set bit and/or
> reviewer name in Trac through API and avoid all changes introduced
> with Patchwork.
>
> Simple script and appropriate Trac query can do better work for us.
>
OK. It looks like it brings more disruption than benefit.
Simo, can we please get back to the original method at least for now?
At this stage of development it see a lot of disruption that we can't
afford.
Patch tracking problem exists - I do not deny it but it seems that the
change to patchwork does not bring the benefits you hoped.
I have an action item to explore what we can do with other tools we
considered like Gerrit. We might be not that far from it becoming an
option. I will follow up and let you know.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
11 years, 7 months
[INI] Remove code that allows duplicate sections
by Dmitri Pal
Hello,
After previous set of patches have been committed I started working on
the last remaining piece - the merge functionality.
Re-reading the code I started some time ago I realized that at the time
I left the question of whether the config object should allow
partitioned section or not was still open.
Consider an example of the config file:
[foo]
x=1
[bar]
y=2
[foo]
z=3
Here we have an example of the case when the section is segmented or if
you look from another POW there are dup sections. So at first I thought
that it is OK to parse the file as is and have the internal config
object have two sections with same name. But after some deep thinking I
realized that it is a bad idea and creates a lot of unnecessary
complexity. So patch removes this code. This paves the way to a more
simple code in the merge function as we can assume that there are no dup
sections in the objects being merged.
I also noticed that doc build is spitting warnings so I opened a
separate ticket about it. That would be my next patch.
https://fedorahosted.org/sssd/ticket/1587
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
11 years, 7 months
Using patchwork for tracking SSSD patch status
by Simo Sorce
Hi all,
a few days ago I installed patchwork and an instance is tracking the
SSSD project here: https://patchwork.acksyn.org/project/SSSD/list/
Although patchwork is not a full cycle review system it seem to work
really well for tacking patches and patchsets and their status.
The only limitation with patchwork is that it really only likes one
patch per email. This is why I switched fully to using git send-email
both for the freeIPA (tracked as well) and the SSSD project.
The advantage of using patchwork and git send-email is that it makes it
easier to track feedback per patch (replies to patches are captured by
patchwork) and because git send-email send the patch as plain email text
content it makes it very easy to comment inline in the patch to
highlight issues.
Some of the SSSD developers already registered into patchwork in order
to manage patches. Please feel free to register, in order to allow sssd
developers to manage patches from anyone I need to explicitly give
maintainership of the project (in patchwork) to your account otherwise
you are allowed only to change status about your own patches (so that
external contributors can handle their own if they so wish).
So feel free to send me an email if you register and need maintainer
status to start changing patch status.
At the moment the patchwork server is not yet hooked into the master git
tree, so it does not detect when patches are finally pushed. So once you
push a password please simply change the status to 'Pushed' (you may
also set the 'archive' bit but it is not required).
You can read more about patchwork following the 'about' link.
If you have any question or issue feel free to reply to this thread or
contact me directly.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11 years, 7 months
[PATCH 0/1] Better debug logging when saving groups
by Stephen Gallagher
There are many places in the sdap_save_group() routine where we could exit due
to failure without logging a detailed message. This patch adds more log
messages in error conditions.
Stephen Gallagher (1):
LDAP: Better debug logging when saving groups
src/providers/ldap/sdap_async_groups.c | 86 +++++++++++++++++++++++++++++-----
1 file changed, 75 insertions(+), 11 deletions(-)
--
1.7.12.1
11 years, 7 months