On 09/16/2016 10:52 AM, Greg Kurz wrote:
> On Fri, 16 Sep 2016 09:37:48 +0200
> Cédric Le Goater <c...@kaod.org> wrote:
>
>> On 09/16/2016 09:19 AM, Greg Kurz wrote:
>>> On Fri, 16 Sep 2016 01:05:11 +0200
>>> Greg Kurz <gr...@kaod.org> wrote:
>>>
>>>> If the call to fid_to_qid() returns an error, we will call v9fs_path_free()
>>>> on uninitialized paths.
>>>>
>>>
>>> I'll add this to the changelog:
>>>
>>> It is a regression introduced by the following commit:
>>>
>>> 56f101ecce0e 9pfs: handle walk of ".." in the root directory
>>>
>>>> Let's fix this by initializing dpath and path before calling fid_to_qid().
>>>>
>>>> Signed-off-by: Greg Kurz <gr...@kaod.org>
>>>> ---
>>>>
>>>> Thanks Paolo (and Coverity) for spotting this.
>>>>
>>>> Cc'ing stable as this is a regression introduced in 2.7. It is also present
>>>> in Michael's stable-2.6-staging branch.
>>>>
>>>> hw/9pfs/9p.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>>>> index dfe293d11d1c..91a497079acb 100644
>>>> --- a/hw/9pfs/9p.c
>>>> +++ b/hw/9pfs/9p.c
>>>> @@ -1320,13 +1320,14 @@ static void v9fs_walk(void *opaque)
>>>> goto out_nofid;
>>>> }
>>>>
>>>> + v9fs_path_init(&dpath);
>>>> + v9fs_path_init(&path);
>>>> +
>>>> err = fid_to_qid(pdu, fidp, &qid);
>>>> if (err < 0) {
>>>> goto out;
>>>> }
>>>>
>>>> - v9fs_path_init(&dpath);
>>>> - v9fs_path_init(&path);
>>>> /*
>>>> * Both dpath and path initially poin to fidp.
>>>> * Needed to handle request with nwnames == 0
>>>>
>>>
>>>
>>
>>
>> There is still a possible segv I think, in out_nofid :
>>
>> if (nwnames && nwnames <= P9_MAXWELEM) {
>> for (name_idx = 0; name_idx < nwnames; name_idx++) {
>> v9fs_string_free(&wnames[name_idx]);
>>
>> &wnames[name_idx] could NULL if pdu_unmarshal() fails.
>>
>
> We're safe because wnames[] is fully allocated and zeroed:
ah yes. I got confused by the structures.
Reviewed-by: Cédric Le Goater <c...@kaod.org>
C.
> if (nwnames && nwnames <= P9_MAXWELEM) {
> wnames = g_malloc0(sizeof(wnames[0]) * nwnames); <--- here
> qids = g_malloc0(sizeof(qids[0]) * nwnames);
> for (i = 0; i < nwnames; i++) {
> err = pdu_unmarshal(pdu, offset, "s", &wnames[i]);
> if (err < 0) {
> goto out_nofid;
> }
>
> But I agree that the calls to v9fs_string_free() without the corresponding
> calls to v9fs_string_init(), as it is done everywhere else, is disturbing
> and should be addressed.
>
> Thanks.
>
> --
> Greg
>
>>
>>
>> C.
>>
>>
>>
>>
>
