Re: [RFC PATCH] bcache: fix stack corruption by PRECEDING_KEY()

[Pierre JUHEN <pierre.juhen@orange.fr>]

Le 09/06/2019 à 02:59, Coly Li a écrit :
> On 2019/6/9 2:50 上午, Rolf Fokkens wrote:
>> On 6/8/19 12:22 PM, Coly Li wrote:
>>> +static inline void preceding_key(struct bkey *k, struct bkey
>>> *preceding_key_p)
>>> +{
>>> +    if (KEY_INODE(k) || KEY_OFFSET(k)) {
>>> +        *preceding_key_p = KEY(KEY_INODE(k), KEY_OFFSET(k), 0);
>>> +        if (!preceding_key_p->low)
>>> +            preceding_key_p->high--;
>>> +        preceding_key_p->low--;
>>> +    } else {
>>> +        preceding_key_p = NULL;
>> If I'm correct, the line above has no net effect, it just changes a
>> local variable (parameter) with no effect elsewhere. So the else part
>> may be left out, or do you mean this?
>>
>> *preceding_key_p = ZERO_KEY;
>>
> Hi Rolf and Pierre,
>
> Setting preceding_key_p to NULL is for the following
> bch_btree_iter_init(). See the call chains
>
> bch_btree_insert_key()->bch_btree_iter_init()->
> __bch_btree_iter_init()->bch_bset_search()
>
> preceding_key_p is parameter 'search' in bch_bset_search().
> If it is NULL, t->data->start returns directly; if it is not NULL,
> __bch_bset_search() is called.
>
> Indeed *preceding_key_p = ZERO_KEY is unnecessary, just makes me
> comfortable. The problem is PRECEDING_KEY() allocates an on-stack
> variable, and this one is overlapped with stackframe of
> bch_btree_iter_init(), and overwritten. Because this anonymous on-stack
> variable is allocated inside PRECEDING_KEY(), not (and should not be)
> protected by compiler.
>
> So I add the new local variable preceding_key (and make preceding_key_p
> points to it) explicitly on stack frame of bch_btree_insert_key(), which
> will never be overlapped with stackframe of bch_btree_iter_init().
>
> Thanks.

HI,


so the right line should be :

*preceding_key_p = NULL;

because Rolf is right

preceding_key_p = NULL;

does change only the value of the calling parameter and exits, not the 
value of the preceding key in the stack.