qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] hw/9pfs/virtio-9p-local.c: use snprintf() inste


From: Chen Gang
Subject: Re: [Qemu-devel] [PATCH] hw/9pfs/virtio-9p-local.c: use snprintf() instead of sprintf()
Date: Tue, 04 Feb 2014 19:22:15 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7

On 02/04/2014 07:06 PM, Daniel P. Berrange wrote:
> On Tue, Feb 04, 2014 at 07:02:18PM +0800, Chen Gang wrote:
>> On 02/03/2014 06:39 PM, Chen Gang wrote:
>>> On 02/03/2014 06:34 PM, Daniel P. Berrange wrote:
>>>> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote:
>>>>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH,
>>>>> so need use snprintf() instead of sprintf().
>>>>>
>>>>> And also recommend to use ARRAY_SIZE instead of hard code macro for an
>>>>> array size in snprintf().
>>>>
>>>> In the event that there is overflow this will cause the data to be
>>>> truncated, potentially causing QEMU to access the wrong file on the
>>>> host. Both snprintf and sprintf are really bad because of their
>>>> use of fixed buffers. Better to change it to g_strdup_printf which
>>>> dynamically allocates buffers.
>>>>
>>
>> After check the details, I guess we can not change to g_strdup_printf or
>> others (e.g. v9fs_string_*).
>>
>> v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if
>> the combined path is longer than MAX_PATH, before it passes to "mkdir,
>> remove ...", it has to be truncated just like what rpath() has done.
> 
> I don't believe you are correct there.  Those functions should
> return "errno == ENAMETOOLONG - pathname was too long". The
> MAX_PATH constant is not even required to exist in POSIX, so
> I would not expect the spec to mandate anything about MAX_PATH
> in relation to those functions.
> 

So the original author of v9fs will use truncation instead of return
failure to upper users.

I guess, we need discuss what original author have done is valuable or
not (at least, it's necessary to mention about it in related documents).

And at least, we are agree with each other: "using sprintf() is a bug".


> Copying Eric who is involved the POSIX spec group to confirm.
> 
> Even if they are limited, it is still better practice to use
> dynamic allocation for this, over fixed length buffers IMHO.
> 
> Daniel
> 

Welcome any members' suggestions, discussion or completions. :-)


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed



reply via email to

[Prev in Thread] Current Thread [Next in Thread]