Re: [Qemu-block] [PATCH] raw-posix.c: Make physical devices usable in QEMU

[Programmingkid]

On Jul 16, 2015, at 9:19 AM, Stefan Hajnoczi wrote:

> On Thu, Jul 09, 2015 at 10:02:26AM -0400, Programmingkid wrote:
>> 
>> On Jul 9, 2015, at 6:52 AM, Stefan Hajnoczi wrote:
>> 
>>> On Tue, Jul 07, 2015 at 01:33:23PM -0400, Programmingkid wrote:
>>>> Make physical devices like a USB flash drive or a CDROM drive work in 
>>>> QEMU. With
>>>> this patch I can use a USB flash drive like a hard drive. Before this 
>>>> patch, 
>>>> QEMU would just quit with a message like "resource busy".
>>> 
>>> The commit message and the description are missing "on Mac OS X".  It
>>> should be clear right away that this applies to Mac only.  This works
>>> fine on Linux and probably other host OSes.
>> 
>> Yeah, that should have been done. Did you see any issues with the code?
> 
> QEMU shouldn't silently open a different file than the one given by the
> user.  The user should give the exact device file they want.  If there
> is magic behavior it needs to be documented, but I don't see a reason
> why that's necessary in the case of device files.

I think you are reviewing an older patch. The newest one doesn't do that. 

> 
> QEMU shouldn't mount/unmount the CD-ROM.  atexit(3) doesn't handle
> crashes or abort().  Users may be confused to find their CD-ROM
> unmounted in those cases and would see this as a bug.  Instead we should
> refuse mounted CD-ROMs so the user understands that block-level access
> requires them to unmount first.

That can be done. It just wouldn't be as user friendly as having QEMU do it for
the user :(

> 
> The strcpy/sprintf usage in this patch is unsafe and can lead to buffer
> overflow, for example in the case of generating command-lines.  The
> command-line buffer is only MAXPATHLEN so prepending the command to the
> filename could exceed the buffer size.
> 
> There is also a buffer overflow in the array of devices that need to be
> mounted.  What happens if there are more than 7 devices?

Ok. Will correct this issue.