Re: [PATCH] {iproute2, xfrm}: Use memcpy to suppress gcc phony buffer overflow warning

[Fan Du <fan.du@windriver.com>]

Back from vacation, sorry for the late reply.

On 2013年09月30日 17:29, David Laight wrote:
>>> This is a false positive warning as the destination pointer "buf"
>>> pointers to
>>> an ZERO length array, which actually will occupy alg.buf mostly.
>>> Fix this by using memcpy.
>>>
>>> struct xfrm_algo {
>>>           char            alg_name[64];
>>>           unsigned int    alg_key_len;    /* in bits */
>>>           char            alg_key[0];
>>> };
>>>
>>> struct {
>>>           union {
>>>                   struct xfrm_algo alg;
>>>                   struct xfrm_algo_aead aead;
>>>                   struct xfrm_algo_auth auth;
>>>           } u;
>>>           char buf[XFRM_ALGO_KEY_BUF_SIZE];
>>> } alg = {};
>>>
>>> buf = alg.u.alg.alg_key;
>
> That is worse than horrid...
> The tools have every right to complain about any accesses to alg_key[].

Only when using strcpy, because a build in checking inserted in this function.

>>> ---
>>>    ip/xfrm_state.c |    2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
>>> index 0d98e78..5cc87d3 100644
>>> --- a/ip/xfrm_state.c
>>> +++ b/ip/xfrm_state.c
>>> @@ -159,7 +159,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg,
>>> enum xfrm_attr_type_t type,
>>>                if (len>  max)
>>>                    invarg("\"ALGO-KEY\" makes buffer overflow\n", key);
>
> I presume there is a return hiding in invarg().

Good guess :)

>>>
>>> -            strncpy(buf, key, len);
>>> +            memcpy(buf, key, len);
>
> Passing the length of the SOURCE to strncpy() is almost always wrong.
> You are still not terminating the copied string.

Don't worry.

The length using here has been increased by 1 at the beginning of the function,
so the copied string to the destination is terminated well.

> 	David
>
>

-- 
浮沉随浪只记今朝笑

--fan