Bug 39283 – Port SharedBuffer to POSIX.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Young Han Lee]

Implement SharedBuffer::createWithContentsOfFile with POSIX functions.

[Young Han Lee]

Created attachment 56356 [details]
patch

[Darin Adler]

Comment on attachment 56356 [details]
patch

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

[Young Han Lee]

(In reply to comment #2)
> (From update of attachment 56356 [details])
> > +    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.

OK. I've done this by new patch (https://bugs.webkit.org/show_bug.cgi?id=39348).
So now, this depends on 39348.

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

I've fixed all above.

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

Oops, I've removed this but don't add other out of memory check. It seems overwork.

> 
> > +    ssize_t bytesRead = 0;
> 
> There's no reason to initialize this to zero. I also suggest defining it after defining totalBytesToRead.

fixed also.

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

Thanks.

[Young Han Lee]

Created attachment 56471 [details]
patch

[Darin Adler]

Comment on attachment 56471 [details]
patch

> +#include "FileSystem.h"
> +
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <wtf/text/CString.h>

We put includes in a single paragraph. Not one for "" and a separate one for <>.

> +    char* filename = filenameFromString(filePath);

Can this function ever fail? If so, then I think we need code to handle that case.

> +    size_t totalBytesRead = 0;
> +    ssize_t bytesRead;
> +    while ((bytesRead = read(fd, result->m_buffer.data() + totalBytesRead, bytesToRead - totalBytesRead)) > 0)
> +        totalBytesRead += bytesRead;

If the length of the file increases due to another process writing to it while this function is running, then this will end up calling read with a size of 0. I suppose that's OK and how you're supposed to use the read call.

[WebKit Commit Bot]

Comment on attachment 56471 [details]
patch

Clearing flags on attachment: 56471

Committed r59890: <http://trac.webkit.org/changeset/59890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.