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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Young Han Lee <joybro at company100.net>  2010-05-19 00:39:53 PST ---
(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.

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