bpc_poolWrite_copyToPool() appears to read and write eight bytes at a time

[setharnold]

rsync-bpc/backuppc/bpc_poolWrite.c

Line 699 in 1b59d68

while ( (nRead = read(fdRead, info->buffer, sizeof(info->buffer))) > 0 ) {

Hello, the function bpc_poolWrite_copyToPool() appears to perform repeated read and write operations eight bytes at a time:

nt bpc_poolWrite_copyToPool(bpc_poolWrite_info *info, char *poolPath, char *fileName)
{
    int fdRead, fdWrite;
    int nRead, nWrite;

    if ( (fdWrite = open(poolPath, O_WRONLY | O_CREAT | O_EXCL, 0666)) < 0 ) {
        info->errorCnt++;
        bpc_logErrf("bpc_poolWrite_copyToPool: can't open/create %s for writing", poolPath);
        return -1;
    }
    if ( (fdRead = open(fileName, O_RDONLY)) < 0 ) {
        info->errorCnt++;
        bpc_logErrf("bpc_poolWrite_copyToPool: can't open %s for reading", fileName);
        return -1;
    }

    while ( (nRead = read(fdRead, info->buffer, sizeof(info->buffer))) > 0 ) {  /* sizeof(pointer) */
        uchar *p = info->buffer;
        int thisWrite;

        nWrite  = 0;
        while ( nWrite < nRead ) {
            do {
                thisWrite = write(fdWrite, p, nRead - nWrite);
            } while ( thisWrite < 0 && errno == EINTR );
...

(Of course on 32-bit platforms it'd be four bytes at a time, but those are dwindling.)

Depending upon how much data is being copied and how often this function is used, this could be very slow. There's a few possible approaches I can think of off the top of my head:

• replace sizeof with BPC_POOL_WRITE_BUF_SZ if the buffer size is well and truly fixed
• use fdopen(3) on these file descriptors to use stdio's buffered io routines
• use fopen(3) on these file names to use stdio's buffered io routines (is mode 0666 correct for the output file? this seems very permissive)

Also, if the fdRead = open(fileName, ...); operation fails, fdWrite could be leaked.

Thanks

[craigbarratt]

Wow - good catch. I should update bpc_poolWrite_info to include the size of the buffer. Although it is always BPC_POOL_WRITE_BUF_SZ it would be better to include it in the struct. This bug happened when I switched from a static fixed buffer in 3.0.9.5.

Yes, there is a missing close on fdWrite in the error case - thanks for noticing that.

On the O_CREAT permissions, I believe rsync has an overall umask, plus the pool files are written to a directory that BackupPC should have the correct permissions on. However, it should be ok to replace the 0666 with 0664.