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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> 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.


That is a bug. The snprintf usage with PATH_MAX is to prevent buffer
overflow  and not to truncate. I guess we should fix path handling
and propagate error correctly.

-aneesh