7:04 a.m.
https://fedorahosted.org/sssd/ticket/1233
As Jakub suggested, I changed sss_sudo_make_request() to return only
errno code, instead of errno and sss_status. I hope this will make the
Coverity happy.
1:57 p.m.
On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1233
As Jakub suggested, I changed sss_sudo_make_request() to return only
errno code, instead of errno and sss_status. I hope this will make the
Coverity happy.
Sorry, that's not exactly what I had mind. I was thinking about
modifying sss_cli_check_socket() and sss_cli_make_request_nochecks() to
not return separate sss_status and errno return codes but only errno.
This patch only modifies the sudo caller of the two functions.
The catch is that especially with NSS calls, some errors are
recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and ERANGE
tells the glibc to retry the lookup with a bigger buffer. So should we
only use one return code, we would have to account for this case, too.
2:17 a.m.
On 03/26/2012 08:57 PM, Jakub Hrozek wrote:
On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1233
>
> As Jakub suggested, I changed sss_sudo_make_request() to return only
> errno code, instead of errno and sss_status. I hope this will make the
> Coverity happy.
Sorry, that's not exactly what I had mind. I was thinking about
modifying sss_cli_check_socket() and sss_cli_make_request_nochecks() to
not return separate sss_status and errno return codes but only errno.
This patch only modifies the sudo caller of the two functions.
The catch is that especially with NSS calls, some errors are
recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and ERANGE
tells the glibc to retry the lookup with a bigger buffer. So should we
only use one return code, we would have to account for this case, too.
Ok. The new patch tests ret instead of errnop and frees repbuf even in
error branch. This should make the Coverity happy.
I didn't found a ticket for what you suggested so I created one:
https://fedorahosted.org/sssd/ticket/1285
3:22 a.m.
On 04/03/2012 09:17 AM, Pavel Březina wrote:
On 03/26/2012 08:57 PM, Jakub Hrozek wrote:
> On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
>> https://fedorahosted.org/sssd/ticket/1233
>>
>> As Jakub suggested, I changed sss_sudo_make_request() to return only
>> errno code, instead of errno and sss_status. I hope this will make the
>> Coverity happy.
>
> Sorry, that's not exactly what I had mind. I was thinking about
> modifying sss_cli_check_socket() and sss_cli_make_request_nochecks() to
> not return separate sss_status and errno return codes but only errno.
> This patch only modifies the sudo caller of the two functions.
>
> The catch is that especially with NSS calls, some errors are
> recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and ERANGE
> tells the glibc to retry the lookup with a bigger buffer. So should we
> only use one return code, we would have to account for this case, too.
Ok. The new patch tests ret instead of errnop and frees repbuf even in
error branch. This should make the Coverity happy.
I didn't found a ticket for what you suggested so I created one:
https://fedorahosted.org/sssd/ticket/1285
Now with the patch attached :-)
11:05 a.m.
On Tue, Apr 03, 2012 at 10:22:37AM +0200, Pavel Březina wrote:
On 04/03/2012 09:17 AM, Pavel Březina wrote:
>On 03/26/2012 08:57 PM, Jakub Hrozek wrote:
>>On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
>>>https://fedorahosted.org/sssd/ticket/1233
>>>
>>>As Jakub suggested, I changed sss_sudo_make_request() to return only
>>>errno code, instead of errno and sss_status. I hope this will make the
>>>Coverity happy.
>>
>>Sorry, that's not exactly what I had mind. I was thinking about
>>modifying sss_cli_check_socket() and sss_cli_make_request_nochecks() to
>>not return separate sss_status and errno return codes but only errno.
>>This patch only modifies the sudo caller of the two functions.
>>
>>The catch is that especially with NSS calls, some errors are
>>recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and ERANGE
>>tells the glibc to retry the lookup with a bigger buffer. So should we
>>only use one return code, we would have to account for this case, too.
>
>Ok. The new patch tests ret instead of errnop and frees repbuf even in
>error branch. This should make the Coverity happy.
>
>I didn't found a ticket for what you suggested so I created one:
>https://fedorahosted.org/sssd/ticket/1285
Now with the patch attached :-)
Ack to the code but can you chance the commit message? It's misleading.
What Pavel fixed in this commit is how error checking is done with
return values of sss_sudo_make_request(), not a memory leak.
2:19 p.m.
On 10.4.2012 18:05, Jakub Hrozek wrote:
On Tue, Apr 03, 2012 at 10:22:37AM +0200, Pavel Březina wrote:
> On 04/03/2012 09:17 AM, Pavel Březina wrote:
>> On 03/26/2012 08:57 PM, Jakub Hrozek wrote:
>>> On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
>>>> https://fedorahosted.org/sssd/ticket/1233
>>>>
>>>> As Jakub suggested, I changed sss_sudo_make_request() to return only
>>>> errno code, instead of errno and sss_status. I hope this will make the
>>>> Coverity happy.
>>>
>>> Sorry, that's not exactly what I had mind. I was thinking about
>>> modifying sss_cli_check_socket() and sss_cli_make_request_nochecks() to
>>> not return separate sss_status and errno return codes but only errno.
>>> This patch only modifies the sudo caller of the two functions.
>>>
>>> The catch is that especially with NSS calls, some errors are
>>> recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and ERANGE
>>> tells the glibc to retry the lookup with a bigger buffer. So should we
>>> only use one return code, we would have to account for this case, too.
>>
>> Ok. The new patch tests ret instead of errnop and frees repbuf even in
>> error branch. This should make the Coverity happy.
>>
>> I didn't found a ticket for what you suggested so I created one:
>> https://fedorahosted.org/sssd/ticket/1285
>
> Now with the patch attached :-)
>
Ack to the code but can you chance the commit message? It's misleading.
What Pavel fixed in this commit is how error checking is done with
return values of sss_sudo_make_request(), not a memory leak.
Patch attached.
3:37 p.m.
On Tue, Apr 10, 2012 at 09:19:13PM +0200, Pavel Březina wrote:
On 10.4.2012 18:05, Jakub Hrozek wrote:
>On Tue, Apr 03, 2012 at 10:22:37AM +0200, Pavel Březina wrote:
>>On 04/03/2012 09:17 AM, Pavel Březina wrote:
>>>On 03/26/2012 08:57 PM, Jakub Hrozek wrote:
>>>>On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
>>>>>https://fedorahosted.org/sssd/ticket/1233
>>>>>
>>>>>As Jakub suggested, I changed sss_sudo_make_request() to return only
>>>>>errno code, instead of errno and sss_status. I hope this will make
the
>>>>>Coverity happy.
>>>>
>>>>Sorry, that's not exactly what I had mind. I was thinking about
>>>>modifying sss_cli_check_socket() and sss_cli_make_request_nochecks() to
>>>>not return separate sss_status and errno return codes but only errno.
>>>>This patch only modifies the sudo caller of the two functions.
>>>>
>>>>The catch is that especially with NSS calls, some errors are
>>>>recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and ERANGE
>>>>tells the glibc to retry the lookup with a bigger buffer. So should we
>>>>only use one return code, we would have to account for this case, too.
>>>
>>>Ok. The new patch tests ret instead of errnop and frees repbuf even in
>>>error branch. This should make the Coverity happy.
>>>
>>>I didn't found a ticket for what you suggested so I created one:
>>>https://fedorahosted.org/sssd/ticket/1285
>>
>>Now with the patch attached :-)
>>
>
>Ack to the code but can you chance the commit message? It's misleading.
>
>What Pavel fixed in this commit is how error checking is done with
>return values of sss_sudo_make_request(), not a memory leak.
Patch attached.
Ack!
9:46 a.m.
On Tue, 2012-04-10 at 16:37 -0400, Jakub Hrozek wrote:
On Tue, Apr 10, 2012 at 09:19:13PM +0200, Pavel Březina wrote:
> On 10.4.2012 18:05, Jakub Hrozek wrote:
> >On Tue, Apr 03, 2012 at 10:22:37AM +0200, Pavel Březina wrote:
> >>On 04/03/2012 09:17 AM, Pavel Březina wrote:
> >>>On 03/26/2012 08:57 PM, Jakub Hrozek wrote:
> >>>>On Fri, Mar 23, 2012 at 01:04:52PM +0100, Pavel Březina wrote:
> >>>>>https://fedorahosted.org/sssd/ticket/1233
> >>>>>
> >>>>>As Jakub suggested, I changed sss_sudo_make_request() to return
only
> >>>>>errno code, instead of errno and sss_status. I hope this will
make the
> >>>>>Coverity happy.
> >>>>
> >>>>Sorry, that's not exactly what I had mind. I was thinking about
> >>>>modifying sss_cli_check_socket() and sss_cli_make_request_nochecks()
to
> >>>>not return separate sss_status and errno return codes but only
errno.
> >>>>This patch only modifies the sudo caller of the two functions.
> >>>>
> >>>>The catch is that especially with NSS calls, some errors are
> >>>>recoverable. Notably the combination of NSS_STATUS_TRYAGAIN and
ERANGE
> >>>>tells the glibc to retry the lookup with a bigger buffer. So should
we
> >>>>only use one return code, we would have to account for this case,
too.
> >>>
> >>>Ok. The new patch tests ret instead of errnop and frees repbuf even in
> >>>error branch. This should make the Coverity happy.
> >>>
> >>>I didn't found a ticket for what you suggested so I created one:
> >>>https://fedorahosted.org/sssd/ticket/1285
> >>
> >>Now with the patch attached :-)
> >>
> >
> >Ack to the code but can you chance the commit message? It's misleading.
> >
> >What Pavel fixed in this commit is how error checking is done with
> >return values of sss_sudo_make_request(), not a memory leak.
>
> Patch attached.
>
Ack!
Pushed to master and sssd-1-8.
4388
days inactive
4414
days old
sssd-devel@lists.fedorahosted.org
7 comments
3 participants
participants (3)
-
Jakub Hrozek -
Pavel Březina -
Stephen Gallagher