in CookieJarWin.cpp, we call InternetGetCookie to get the length of the buffer, then call it again once a buffer of the correct size has been constructed. However, the cookies can change between these values if another process (or possibly some other cases, I'm not sure) changes the cookies for the page between these calls. In particular, if the cookies are deleted, we will make a string of length -1, causing a crash. If more cookies are added, the returned cookies will be truncated.
Created attachment 12172 [details] Patch to CookieJarWin.cpp This patch requests the cookies in a loop until the buffer is large enough to hold the cookies. As long as the length of the cookies doesn't grow monotonically, this will terminate.
This patch still causes crashes. InternetGetCookie is buggy and will write one past the end of the buffer on ERROR_INVALID_PARAMETER. At the top of the loop: count = cookieBuffer.size() - 1; and at the bottom, we need: cookieBuffer.resize(count + 1); to compensate.
Is this still a problem?
In Safari on Windows, this is not a problem. CookieJarWin.cpp is still in the tree, though, and it still has this bug, it just isn't used by anybody. Probably that file should just be deleted.
(In reply to comment #4) > In Safari on Windows, this is not a problem. CookieJarWin.cpp is still in the > tree, though, and it still has this bug, it just isn't used by anybody. > Probably that file should just be deleted. A patch to delete that file (and others not currently being used) would be welcome. There's no point keeping unused files in trunk, and they can always be resurrected from an old revision if desired.
It looks like this file is now used by the new Windows port. The buggy code is still there, though.
(In reply to comment #6) > It looks like this file is now used by the new Windows port. The buggy code is > still there, though. ...but it sounds like we haven't seen this to be a problem in practice with the CFNetwork cookies implementation. Would still be nice to fix sometime, but it's not urgent.
Based on https://trac.webkit.org/changeset/176672 , this bug is no longer valid.