12081 2007-01-02 13:25:39 -0800 Crash in Windows cookie code if cookies change between calls 2017-02-28 20:38:49 -0800 1 1 1 Unclassified WebKit Platform 420+ PC Windows XP RESOLVED WONTFIX P2 Normal --- 1 brettw webkit-unassigned ap aroben basuke bugs-webkit fujii oldest_to_newest 37659 0 brettw 2007-01-02 13:25:39 -0800 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. 37645 1 12172 brettw 2007-01-02 13:31:00 -0800 Created attachment 12172 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. 33921 2 brettw 2007-01-21 11:32:37 -0800 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. 4741 3 ap 2007-07-15 01:13:13 -0700 Is this still a problem? 3146 4 brettw 2007-08-02 08:53:53 -0700 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. 3128 5 aroben 2007-08-02 11:59:24 -0700 (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. 64848 6 brettw 2007-12-20 11:22:15 -0800 It looks like this file is now used by the new Windows port. The buggy code is still there, though. 66023 7 aroben 2008-01-02 06:50:24 -0800 (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. 1281971 8 basuke 2017-02-28 15:01:48 -0800 Based on https://trac.webkit.org/changeset/176672 , this bug is no longer valid. 12172 2007-01-02 13:31:00 -0800 2010-06-10 15:22:02 -0700 Patch to CookieJarWin.cpp Cookie.diff text/plain 1977 brettw SW5kZXg6IENvb2tpZUphcldpbi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gQ29va2llSmFyV2luLmNwcAko cmV2aXNpb24gMTgyMzMpCisrKyBDb29raWVKYXJXaW4uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0y Nyw2ICsyNyw3IEBACiAjaW5jbHVkZSAiS1VSTC5oIgogI2luY2x1ZGUgIlBsYXRmb3JtU3RyaW5n LmgiCiAjaW5jbHVkZSAiRGVwcmVjYXRlZFN0cmluZy5oIgorI2luY2x1ZGUgIlZlY3Rvci5oIgog I2luY2x1ZGUgPHdpbmRvd3MuaD4KICNpbmNsdWRlIDxXaW5pbmV0Lmg+CiAKQEAgLTQ3LDE3ICs0 OCwzMSBAQAogewogICAgIERlcHJlY2F0ZWRTdHJpbmcgc3RyID0gdXJsLnVybCgpOwogICAgIHN0 ci5hcHBlbmQoKFVDaGFyKSdcMCcpOworICAgIFVDaGFyKiB1cmxfY2hhciA9IChVQ2hhciopc3Ry LnVuaWNvZGUoKTsKIAotICAgIERXT1JEIGNvdW50OwotICAgIEludGVybmV0R2V0Q29va2llKChV Q2hhciopc3RyLnVuaWNvZGUoKSwgMCwgMCwgJmNvdW50KTsKLSAgICBpZiAoY291bnQgPD0gMSkg Ly8gTnVsbCB0ZXJtaW5hdG9yIGNvdW50cyBhcyAxLgotICAgICAgICByZXR1cm4gU3RyaW5nKCk7 CisgICAgLy8gQ29va2llcyBjYW4gY2hhbmdlIGF0IGFueSB0aW1lLCBpbmNsdWRpbmcgYmV0d2Vl biBjYWxscyB0bworICAgIC8vIEludGVybmV0R2V0Q29va2llLCBzbyB3ZSd2ZSBzZXQgdXAgYSBy YWNlIGNvbmRpdGlvbi4gV2UgdGhlcmVmb3JlIGRvCisgICAgLy8gYSBsb29wIHVudGlsIHdlIGZp bmFsbHkgZ2V0IGEgYnVmZmVyIGJpZyBlbm91Z2ggdG8gaG9sZCB3aGF0ZXZlciB0aGUKKyAgICAv LyBjdXJyZW50IGNvbnRlbnRzIG9mIHRoZSBjb29raWVzIGFyZS4KKyAgICBjb25zdCBpbnQgaW5p dGFsX3NpemUgPSAxMDI0OyAvLyBoYXZlIHZlY3RvciBzdGFjayBhbGxvY2F0ZSB0aGlzIG11Y2gK KyAgICBXVEY6OlZlY3RvcjxVQ2hhciwgaW5pdGFsX3NpemU+IGNvb2tpZUJ1ZmZlcihpbml0YWxf c2l6ZSk7CisgICAgd2hpbGUgKHRydWUpCisgICAgeworICAgICAgICBEV09SRCBjb3VudCA9IGNv b2tpZUJ1ZmZlci5zaXplKCk7CisgICAgICAgIGlmIChJbnRlcm5ldEdldENvb2tpZSh1cmxfY2hh ciwgMCwgY29va2llQnVmZmVyLmRhdGEoKSwgJmNvdW50KSkgeworICAgICAgICAgICAgLy8gZ290 IGNvb2tpZXMgYW5kIHRoZXkgZml0IGluIG91ciBidWZmZXIKKyAgICAgICAgICAgIGlmIChjb3Vu dCA8PSAxKSAKKyAgICAgICAgICAgICAgICByZXR1cm4gU3RyaW5nKCk7IC8vIEVtcHR5IChudWxs IHRlcm1pbmF0b3IgY291bnRzIGFzIDEpCisgICAgICAgICAgICByZXR1cm4gU3RyaW5nKGNvb2tp ZUJ1ZmZlci5kYXRhKCksIGNvdW50IC0gMSk7IC8vIGlnbm9yZSBOVUxMIHRlcm1pbmF0b3IKKyAg ICAgICAgfQogCi0gICAgVUNoYXIqIGJ1ZmZlciA9IG5ldyBVQ2hhcltjb3VudF07Ci0gICAgSW50 ZXJuZXRHZXRDb29raWUoKFVDaGFyKilzdHIudW5pY29kZSgpLCAwLCBidWZmZXIsICZjb3VudCk7 Ci0gICAgU3RyaW5nJiByZXN1bHQgPSBTdHJpbmcoYnVmZmVyLCBjb3VudC0xKTsgLy8gSWdub3Jl IHRoZSBudWxsIHRlcm1pbmF0b3IuCi0gICAgZGVsZXRlW10gYnVmZmVyOwotICAgIHJldHVybiBy ZXN1bHQ7CisgICAgICAgIC8vIFRyZWF0IGZhaWx1cmVzIG90aGVyIHRoYW4gaW5zdWZmaWNpZW50 IGJ1ZmZlciBhcyBub3QgaGF2aW5nIGFueSBjb29raWVzCisgICAgICAgIGlmIChHZXRMYXN0RXJy b3IoKSAhPSBFUlJPUl9JTlNVRkZJQ0lFTlRfQlVGRkVSKQorICAgICAgICAgICAgcmV0dXJuIFN0 cmluZygpOworCisgICAgICAgIC8vIGJ1ZmZlciBub3QgbGFyZ2UgZW5vdWdoLCBydW4gbG9vcCBh Z2FpbiB3aXRoIGxhcmdlciBidWZmZXIKKyAgICAgICAgY29va2llQnVmZmVyLnJlc2l6ZShjb3Vu dCk7CisgICAgfQogfQogCiBib29sIGNvb2tpZXNFbmFibGVkKCkK