On 11/14/2013 01:14 PM, Lukas Slebodnik wrote:> On (05/11/13 15:18),
Michal Židek wrote:
>>>> Thanks for the responses.
>>>>
>>>> I decided to reduce the patches for this thread to only
>>>> similar or simple changes. I think it is still quite enough
>>>> for a review. I will post patches for the rest of the issues
>>>> (like the one with
sockaddr) in
>>>> separate threads. I hope this will make review
easier. So
>>>> after
applying
>>>> these patches there will still be a lot of warnings.
>>>>
>>>> New patches attached.
>>>>
>>>> PS: Sorry for delayed answer.
>>>>
>>>>
>>
>> Sending patches rebased on top of current master.
>>
>> Michal
>>
> From 93a3b127e24ed26068ca7cfea41992a86440d8c2 Mon Sep 17 00:00:00
> 2001
>> From: Michal Zidek<mzidek(a)redhat.com> Date: Wed, 21 Aug 2013
>> 17:17:06 +0200 Subject: [PATCH 1/7] sss_client: Use
>> SAFEALIGN_COPY_<type> macros where diff --git
>> a/src/sss_client/idmap/sss_nss_idmap.c
b/src/sss_client/idmap/sss_nss_idmap.c
>> index e0faf6e..69f825f 100644 ---
>> a/src/sss_client/idmap/sss_nss_idmap.c +++
>> b/src/sss_client/idmap/sss_nss_idmap.c @@ -108,7 +108,7 @@ static
>> int sss_nss_getyyybyxxx(union input inp,
enum sss_cli_command cmd ,
>> goto done; }
>>
>> - num_results = ((uint32_t *)repbuf)[0]; +
>> SAFEALIGN_COPY_UINT32(&num_results, repbuf, NULL); if
>> (num_results == 0) { ret = ENOENT; goto done; @@ -117,7 +117,7 @@
>> static int sss_nss_getyyybyxxx(union input inp,
enum sss_cli_command cmd ,
>> goto done; }
>>
Please add comment why comment why you skipped one unit32_t There is
already similar comment on the other place in this patch. /* Skip
first two 32 bit values (number of results and * reserved padding)
*/
Added.
>> - out->type = ((uint32_t *)repbuf)[2]; +
>> SAFEALIGN_COPY_UINT32(&out->type, repbuf + 2 *
sizeof(uint32_t),
NULL);
>>
>> data_len = replen - DATA_START;
>>
>> diff --git a/src/sss_client/nss_group.c
>> b/src/sss_client/nss_group.c index e6ea54b..a7fb093 100644 ---
>> a/src/sss_client/nss_group.c +++ b/src/sss_client/nss_group.c @@
>> -298,7 +298,7 @@ enum nss_status _nss_sss_initgroups_dyn(const
char *user,
gid_t group,
>> }
>>
>> /* no results if not found */ - num_ret = ((uint32_t
>> *)repbuf)[0]; + SAFEALIGN_COPY_UINT32(&num_ret, repbuf,
>> NULL); if (num_ret == 0) { free(repbuf); nret =
>> NSS_STATUS_NOTFOUND; @@ -328,9 +328,13 @@ enum nss_status
>> _nss_sss_initgroups_dyn(const
char *user, gid_t group,
>> *size = newsize; }
>>
The same issue is here.
>> - rbuf = &((uint32_t *)repbuf)[2]; + /* Skip first two 32
>> bit values (number of results and + * reserved padding) */
^^^^^^^^^^^^^^^^^^^^^
The comment was there. Maybe I missed something?
>> + buf_index = 2 * sizeof(uint32_t); + for (l = 0; l <
max_ret;
>> l++) { - (*groups)[*start] = rbuf[l]; +
>> SAFEALIGN_COPY_UINT32(&(*groups)[*start], repbuf + buf_index, +
>> &buf_index); *start += 1; }
>>
Otherwise, conditions look much nicer with variables like a
"num_results" instead of "buffer magic" (((uint32_t *)repbuf)[0] ==
0)
New patch attached.
Michal