Bug 12081 – Crash in Windows cookie code if cookies change between calls

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Platform

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Alexey Proskuryakov]

Is this still a problem?

[Brett Wilson (Google)]

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.

[Adam Roben (:aroben)]

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

[Brett Wilson (Google)]

It looks like this file is now used by the new Windows port. The buggy code is still there, though.

[Adam Roben (:aroben)]

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

[Basuke Suzuki]

Based on https://trac.webkit.org/changeset/176672 , this bug is no longer valid.