[webkit-reviews] review denied: [Bug 207972] Make support for bytecode caching more robust against file corruption. : [Attachment 391224] proposed patch.

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 207972: Make support for bytecode caching more robust against file
corruption.
https://bugs.webkit.org/show_bug.cgi?id=207972

Attachment 391224: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=391224&action=review




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 391224
  --> https://bugs.webkit.org/attachment.cgi?id=391224
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=391224&action=review

Thanks for the feedback.  After thinking about this more, I decided to drop (3)
since its benefit to risk ratio is low.

>> Source/JavaScriptCore/ChangeLog:33
>> +	       header current only contains a fileSize value.
> 
> nit: current -> currently

Removed with (3).

>> Source/JavaScriptCore/API/JSScript.mm:153
>> +	uint32_t unusedReservedForFutureUse[3];
> 
> Do we actually need this? And if we need this to be able to add things at the
beginning of the cache file, why can we add things in this patch?
> If it is purely for alignment purposes, I would maybe change the name/type to
something like uint8_t paddingForAlignment[12];

Removed with (3).

>> Source/JavaScriptCore/API/JSScript.mm:282
>> +	int fd = open(tokenFileName, O_CREAT | O_NONBLOCK, 0666);
> 
> What is the minimum permission required for checking if a file exist and
deleting it?
> Allowing anyone to write to this file is probably not too bad (since no-one
looks at it), but it feels unnecessary, and might maybe be useful for an
attacker trying to persist information across reboots.

I've change the 0666 to 0.

>> Source/JavaScriptCore/API/JSScript.mm:337
>> +	const char* tempFileName = [cachePathString
stringByAppendingString:@".tmp"].UTF8String;
> 
> Nit: discrepancy between tempFileName and cacheFilename.
> Suggested name change: cacheFilename => cacheFileName

Fixed.

>> Source/JavaScriptCore/API/JSScript.mm:338
>> +	int fd = open(cacheFilename, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK,
0666);
> 
> 666 seems excessive, see above
> Also, we seem not to ever use fd? Since we just rename tempFileName to
cacheFilename

Changed to O_WRONLY with 0600.

>> Source/JavaScriptCore/API/JSScript.mm:344
>> +	int tempFD = open(tempFileName, O_CREAT | O_RDWR | O_EXLOCK |
O_NONBLOCK, 0666);
> 
> 666 seems excessive, see above
> Also, do we need O_RDWR, or just O_WRONLY ?

Changed to O_RDWR and 0600.  O_RDWR is actually needed.  The renaming of the
temp file to cache file fails without it.

>> Source/JavaScriptCore/runtime/CodeCache.h:158
>> +	    sourceProvider.notifyBeforeReadingCachedBytecode();
> 
> Should this not be before "sourceProvider.cachedBytecode()", to also catch
crashes in that code?
> Also I am a bit surprised that this pattern of
notifyBeforeReadingCachedBytecode() -> do stuff ->
notifyAfterReadingCachedBytecode() happens twice in this patch, I would have
expected reading the cache to be done in a single place.

Not really.  The cachedBytecode() method only returns a RefPtr<CachedBytecode>
which is a container object that will point to the mapped file buffer.	It does
not read anything from the file itself.