[Webkit-unassigned] [Bug 39283] Port SharedBuffer to POSIX.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39283


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56356|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2010-05-18 09:19:33 PST ---
(From update of attachment 56356)
> +    int fd = open(filePath.utf8().data(), O_RDONLY);

This is not a portable way to convert a WebCore string to a POSIX filename. See the functions named filenameFromString in the various FileSystem source files to see how it's done on various platforms. We may need to add something to FileSystem.h so we can do this in a way that works across platforms.

> +    size_t bytesToRead = fileStat.st_size;

If the file size is greater than what can fit in size_t, this assignment will truncate the size. The type of st_size is off_t, and there's no guarantee it can fit in a size_t. We should include some sort of check here to make sure the function returns 0 in a case like that instead of reading only part of the file.

> +    RefPtr<SharedBuffer> result = SharedBuffer::create();

Here in the SharedBuffer class, this function should just be called create(). See examples in other member functions.

> +    result->m_buffer.resize(bytesToRead);

It's slightly better to use the grow function here instead of the resize function, since it won't do unnecessary checks related to shrinking a buffer and here we know we are either growing it or leaving it as-is.

> +    if (result->m_buffer.size() != bytesToRead) {
> +        close(fd);
> +        return 0;
> +    }

This code path contains dead code and thus can't be tested. The resize function can't fail. The size is guaranteed to be equal to bytesToRead. The resize function will call CRASH if it can't allocate memory. If we want a code path that will return 0 if we are out of memory then we need to make use of the tryReserveCapacity function, a function that can fail without calling CRASH. Then check for failure, and then call the grow function.

> +    ssize_t bytesRead = 0;

There's no reason to initialize this to zero. I also suggest defining it after defining totalBytesToRead.

review- because at least some of the issues I mention above should be resolved

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.