Re: [Qemu-devel] [PATCH v3] raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Stefan Hajnoczi]

On Fri, Jul 17, 2015 at 08:19:16PM -0400, Programmingkid wrote:
> @@ -2014,7 +2015,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, 
> char *bsdPath, CFIndex ma
>          if ( bsdPathAsCFString ) {
>              size_t devPathLength;
>              strcpy( bsdPath, _PATH_DEV );
> -            strcat( bsdPath, "r" );
> +            if (flags & BDRV_O_NOCACHE) {
> +                strcat(bsdPath, "r");
> +            }
>              devPathLength = strlen( bsdPath );
>              if ( CFStringGetCString( bsdPathAsCFString, bsdPath + 
> devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
>                  kernResult = KERN_SUCCESS;

This hunk should be a separate patch.  It fixes raw-posix alignment
probing by only using the raw char device (which has alignment
constraints) if BDRV_O_NOCACHE was given.

> @@ -2027,7 +2030,35 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, 
> char *bsdPath, CFIndex ma
>      return kernResult;
>  }
>  
> -#endif
> +/* Sets up a real cdrom for use in QEMU */
> +static bool setupCDROM(char *bsdPath)
> +{
> +    int index, numOfTestPartitions = 2, fd;
> +    char testPartition[MAXPATHLEN];
> +    bool partitionFound = false;
> +
> +    /* look for a working partition */
> +    for (index = 0; index < numOfTestPartitions; index++) {
> +        pstrcpy(testPartition, strlen(bsdPath)+1, bsdPath);

The safe way to use pstrcpy is:

char dest[LEN];
pstrcpy(dest, sizeof(dest), src);

Use the destination buffer size since that's what needs to be checked to
prevent buffer overflow.

Using the source buffer size could cause an overflow if the source
buffer is larger than the destination buffer.  Even if that's not the
case today, it's bad practice because it could lead to bugs if code is
modified.

> +        snprintf(testPartition, MAXPATHLEN, "%ss%d", testPartition, index);

Using the same buffer as the destination and a format string argument is
questionable.  I wouldn't be surprised if some snprintf()
implementations produce garbage when you make them read from the same
buffer they are writing to.

Please replace pstrcpy() and snprintf() with a single call:

snprintf(testPartition, sizeof(testPartition), "%ss%d", bsdPath, index);

> +        fd = qemu_open(testPartition, O_RDONLY | O_BINARY | O_LARGEFILE);
> +        if (fd >= 0) {
> +            partitionFound = true;
> +            qemu_close(fd);
> +            break;
> +        }
> +    }
> +
> +    /* if a working partition on the device was not found */
> +    if (partitionFound == false) {
> +        printf("Error: Failed to find a working partition on disc!\n");
> +    } else {
> +        DPRINTF("Using %s as optical disc\n", testPartition);
> +        pstrcpy(bsdPath, strlen(testPartition)+1, testPartition);

Please use MAXPATHLEN instead of strlen(testPartition)+1.

pgpVSTJGH6GUp.pgp
Description: PGP signature