101649 2012-11-08 14:53:00 -0800 m_columnLogicalWidthChanged is manipulating uninitialized memory 2022-12-01 08:59:06 -0800 1 1 1 Unclassified WebKit Tables 528+ (Nightly build) All All NEW P2 Normal --- 1 jchaffraix jchaffraix ahmad.saleem792 ap cdumez darin ddkilzer eric mjs webkit.review.bot oldest_to_newest 762211 0 jchaffraix 2012-11-08 14:53:00 -0800 Because Vector<int> doesn't initialize the memory it allocates, m_columnPos contains uninitialized memory that we query before setting it in RenderTable::setColumnPosition: m_columnLogicalWidthChanged |= m_columnPos[index] != position; As we use the information only to propagate some layout bits to the table parts, this bug is not security sensitive. 762242 1 173134 jchaffraix 2012-11-08 15:24:19 -0800 Created attachment 173134 Proposed memory fix: initialize m_columnPos. 762390 2 ap 2012-11-08 18:05:15 -0800 (In reply to comment #0) > Because Vector<int> doesn't initialize the memory it allocates Is this accurate? explicit Vector(size_t size) : m_size(size) , m_buffer(size) { if (begin()) TypeOperations::initialize(begin(), end()); 763133 3 jchaffraix 2012-11-09 10:17:39 -0800 (In reply to comment #2) > (In reply to comment #0) > > Because Vector<int> doesn't initialize the memory it allocates > > Is this accurate? > > explicit Vector(size_t size) > : m_size(size) > , m_buffer(size) > { > if (begin()) > TypeOperations::initialize(begin(), end()); The VectorTraits magic resolves what TypeOperations::initialize does. For ints, the logic determines that we don't need an initialization (see below) so it is a no-op. template<typename T> struct VectorTraits : VectorTraitsBase<IsPod<T>::value, T> { }; template<typename T> struct VectorTraitsBase<true, T> { static const bool needsInitialization = false; ... } I didn't link to the Chromium bug but this was reported by Valgrind and Memory Sanitizer. 763141 4 ap 2012-11-09 10:26:17 -0800 Hrm, I wonder why this behavior was chosen for. My understanding is that this does not match std::vector behavior, and is thus likely to lead to bugs like this one. 763425 5 mjs 2012-11-09 16:11:49 -0800 (In reply to comment #4) > Hrm, I wonder why this behavior was chosen for. My understanding is that this does not match std::vector behavior, and is thus likely to lead to bugs like this one. It was for efficiency, to avoid zero-filling in cases where it seems unnecessary. POD types don't get initialized at all. We could choose to zero-fill them instead if that seems like a better tradeoff. In particular, if it had no effect on performance to change template<typename T> struct VectorTraitsBase<true, T> (the VectorTraits for POD types) to set needsInitialization to true and canInitializeWithMemset to true, then we should definitely do it. If it does cause a perf regression, we'd have to figure out how we can avoid it. 764673 6 173134 eric 2012-11-12 12:36:18 -0800 Comment on attachment 173134 Proposed memory fix: initialize m_columnPos. I'm confused how this is covered by existing tests? This only shows up in valgrind? 764816 7 173134 jchaffraix 2012-11-12 14:16:54 -0800 Comment on attachment 173134 Proposed memory fix: initialize m_columnPos. (In reply to comment #6) > (From update of attachment 173134 [details]) > I'm confused how this is covered by existing tests? This only shows up in valgrind? This bug only shows up under Valgrind. What I meant is that it reproduces with our existing tests so we don't need extra testing. Clearing the review flag until I can explore the initialize POD by default option. Note that this patch avoids the issue but may not be as generalized as it could. 1915738 8 ahmad.saleem792 2022-11-30 15:35:34 -0800 Fixed in Blink (2014) but didn't have any test case - https://src.chromium.org/viewvc/blink?view=revision&revision=174384 https://github.com/WebKit/WebKit/blob/8e087fe5157f63a97276f5f3155ff14974929491/Source/WTF/wtf/Vector.h#L73 etc. Is this needed now? Thanks! 1915898 9 darin 2022-12-01 04:53:16 -0800 Change doesn’t seem particularly valuable to me unless it’s actively preventing someone from using a memory analysis tool. The code in the change is a bit inelegant. 173134 2012-11-08 15:24:19 -0800 2012-11-12 14:16:54 -0800 Proposed memory fix: initialize m_columnPos. bug-101649-20121108152228.patch text/plain 4372 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzNDUwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjYwM2FiYjRjMjMyODZi ZGE3OTc1NmU2NWU3YzU1ZWY4ODgxNWQxNi4uMTkwMzE1NGExMDk0MjkzNzMwYmZmMzVmNDE4ZGMx YWZjOTU4NzI1MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDMwIEBACisyMDEyLTExLTA4ICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIG1fY29sdW1u TG9naWNhbFdpZHRoQ2hhbmdlZCBpcyBtYW5pcHVsYXRpbmcgdW5pbml0aWFsaXplZCBtZW1vcnkK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMTY0OQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFZlY3Rvcjxp bnQ+IGRvZXNuJ3QgaW5pdGlhbGl6ZSB0aGUgbWVtb3J5IGl0IGFsbG9jYXRlcyBmb3Igc3BlZWQu IFRoZSBsb2dpYyBhZGRlZCBpbgorICAgICAgICByMTMxNDY1IHdvdWxkIGVuZCB1cCByZWFkaW5n IHNvbWUgbmV3bHkgYWxsb2NhdGVkIG1lbW9yeSBiZWZvcmUgd2UgaW5pdGlhbGl6ZSBpdC4gVGhp cworICAgICAgICBjaGFuZ2UgbWFrZXMgdGhlIGRpZmZlcmVudCBmdW5jdGlvbnMgd2hlcmUgd2Ug Z3JvdyBtX2NvbHVtblBvcyBwcm9wZXJseSBpbml0aWFsaXplIGl0LgorCisgICAgICAgIFJlZmFj dG9yaW5nLCBjb3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3QuCisKKyAgICAgICAgKiByZW5kZXJpbmcv UmVuZGVyVGFibGUuaDoKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyVGFibGUuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6UmVuZGVyVGFibGU6OnJlc2l6ZUNvbHVtbnMpOgorICAgICAgICBOZXcg ZnVuY3Rpb24gdXNlZCB0byByZXNpemUgYm90aCBjb2x1bW5zIFZlY3RvcnMuCisgICAgICAgIE5v dGU6IFRoZSBmdW5jdGlvbiBpcyB8Y29uc3R8IGFzIHJlY2FsY1NlY3Rpb25zIGlzIHxjb25zdHwu IEl0J3MgcHJldHR5IGR1bWIgdGhvdWdoLgorCisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJUYWJs ZTo6cmVjYWxjU2VjdGlvbnMpOiBVcGRhdGVkIHRvIGNhbGwgcmVzaXplQ29sdW1ucy4KKworICAg ICAgICAoV2ViQ29yZTo6UmVuZGVyVGFibGU6OnNwbGl0Q29sdW1uKToKKyAgICAgICAgKFdlYkNv cmU6OlJlbmRlclRhYmxlOjphcHBlbmRDb2x1bW4pOgorICAgICAgICBDaGFuZ2VkIHRvIGF2b2lk IGNhbGxzIHRvIGdyb3cuIFRoZSBsb2dpYyBqdXN0IHJlcGxpY2F0ZXMgdGhlIHByZXZpb3VzIGNv bHVtbiwgd2hpY2gKKyAgICAgICAgaXMgbm90IHN0cmljdGx5IE9LIGZvciB0aGUgbGFzdCBwb3Np dGlvbiBhcyBpdCByZXByZXNlbnRzIHRoZSBsYXN0IGNvbHVtbnMnIGVuZC4KKyAgICAgICAgSG93 ZXZlciBpdCBpcyBhIHNpbXBsZSBjaG9pY2UgZm9yIGEgbG9naWNhbCBpbml0aWFsIHZhbHVlLgor CiAyMDEyLTExLTA1ICBTaW1vbiBIYXVzbWFubiAgPHNpbW9uLmhhdXNtYW5uQGRpZ2lhLmNvbT4K IAogICAgICAgICBVbnJldmlld2VkIHRyaXZpYWwgUXQgYnVpbGQgZml4LgpkaWZmIC0tZ2l0IGEv U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRhYmxlLmNwcCBiL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJUYWJsZS5jcHAKaW5kZXggOTQ4YjUyNmQzODA5Yjc4YTI4ZDkwZjli MmQ3ZWIxMjdkMmE5M2U3MS4uZGRmZTg5NDUwMmI3Y2M5MDg3NzUwNGFhOWFhOWEzNGUyZTJlMWFm ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRhYmxlLmNwcAor KysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGUuY3BwCkBAIC0zNjAsNiAr MzYwLDI2IEBAIHZvaWQgUmVuZGVyVGFibGU6OmRpc3RyaWJ1dGVFeHRyYUxvZ2ljYWxIZWlnaHQo aW50IGV4dHJhTG9naWNhbEhlaWdodCkKICAgICAvLyBBU1NFUlQoIXRvcFNlY3Rpb24oKSB8fCAh ZXh0cmFMb2dpY2FsSGVpZ2h0KTsKIH0KIAordm9pZCBSZW5kZXJUYWJsZTo6cmVzaXplQ29sdW1u cyh1bnNpZ25lZCBuZXdTaXplKSBjb25zdAoreworICAgIHVuc2lnbmVkIG9sZFNpemUgPSBtX2Nv bHVtbnMuc2l6ZSgpOworICAgIGlmIChuZXdTaXplIDw9IG9sZFNpemUpIHsKKyAgICAgICAgbV9j b2x1bW5zLnNocmluayhuZXdTaXplKTsKKyAgICAgICAgbV9jb2x1bW5Qb3Muc2hyaW5rKG5ld1Np emUgKyAxKTsKKyAgICAgICAgcmV0dXJuOworICAgIH0KKworICAgIG1fY29sdW1ucy5ncm93KG5l d1NpemUpOworICAgIG1fY29sdW1uUG9zLmdyb3cobmV3U2l6ZSArIDEpOworCisgICAgLy8gVmVj dG9yPGludD4gZG9lc24ndCBpbml0aWFsaXplIHRoZSBhbGxvY2F0ZWQgbWVtb3J5LiBzZXRDb2x1 bW5Mb2dpY2FsV2lkdGggY29tcGFyZXMgdGhlIG9sZAorICAgIC8vIGFuZCB0aGUgbmV3IHZhbHVl cyBvZiBtX2NvbHVtblBvcywgd2hpY2ggbWVhbnMgdGhhdCB3ZSBuZWVkIHRvIGdpdmUgdGhlbSBz b21lIHNlbnNpYmxlIGluaXRpYWwKKyAgICAvLyB2YWx1ZS4gVGhlIG5ldyBjb2x1bW5zIGFyZSAw cHggYXMgd2UgY29weSB0aGUgbGFzdCBjb2x1bW4gc3RhcnQgcG9zaXRpb24sIHdlIG1vdmUgdGhl IGxhc3QKKyAgICAvLyBjb2x1bW4ncyBlbmQgcG9zaXRpb24gYWNjb3JkaW5nbHkuCisgICAgbV9j b2x1bW5Qb3NbbmV3U2l6ZV0gPSBtX2NvbHVtblBvc1tvbGRTaXplXTsKKyAgICBtZW1zZXQoJm1f Y29sdW1uUG9zW29sZFNpemVdLCBvbGRTaXplID8gbV9jb2x1bW5Qb3Nbb2xkU2l6ZSAtIDFdIDog MCwgbmV3U2l6ZSAtIG9sZFNpemUpOworfQorCiB2b2lkIFJlbmRlclRhYmxlOjpsYXlvdXQoKQog ewogICAgIFN0YWNrU3RhdHM6OkxheW91dENoZWNrUG9pbnQgbGF5b3V0Q2hlY2tQb2ludDsKQEAg LTczNCw3ICs3NTQsNyBAQCB2b2lkIFJlbmRlclRhYmxlOjpzcGxpdENvbHVtbih1bnNpZ25lZCBw b3NpdGlvbiwgdW5zaWduZWQgZmlyc3RTcGFuKQogICAgICAgICBzZWN0aW9uLT5zcGxpdENvbHVt bihwb3NpdGlvbiwgZmlyc3RTcGFuKTsKICAgICB9CiAKLSAgICBtX2NvbHVtblBvcy5ncm93KG51 bUVmZkNvbHMoKSArIDEpOworICAgIG1fY29sdW1uUG9zLmluc2VydChwb3NpdGlvbiwgbV9jb2x1 bW5Qb3NbcG9zaXRpb25dKTsKICAgICBzZXROZWVkc0xheW91dEFuZFByZWZXaWR0aHNSZWNhbGMo KTsKIH0KIApAQCAtNzU2LDcgKzc3Niw3IEBAIHZvaWQgUmVuZGVyVGFibGU6OmFwcGVuZENvbHVt bih1bnNpZ25lZCBzcGFuKQogICAgICAgICBzZWN0aW9uLT5hcHBlbmRDb2x1bW4obmV3Q29sdW1u SW5kZXgpOwogICAgIH0KIAotICAgIG1fY29sdW1uUG9zLmdyb3cobnVtRWZmQ29scygpICsgMSk7 CisgICAgbV9jb2x1bW5Qb3MuYXBwZW5kKG1fY29sdW1uUG9zLmxhc3QoKSk7CiAgICAgc2V0TmVl ZHNMYXlvdXRBbmRQcmVmV2lkdGhzUmVjYWxjKCk7CiB9CiAKQEAgLTg3NSwxMCArODk1LDggQEAg dm9pZCBSZW5kZXJUYWJsZTo6cmVjYWxjU2VjdGlvbnMoKSBjb25zdAogICAgICAgICAgICAgICAg IG1heENvbHMgPSBzZWN0aW9uQ29sczsKICAgICAgICAgfQogICAgIH0KLSAgICAKLSAgICBtX2Nv bHVtbnMucmVzaXplKG1heENvbHMpOwotICAgIG1fY29sdW1uUG9zLnJlc2l6ZShtYXhDb2xzICsg MSk7CiAKKyAgICByZXNpemVDb2x1bW5zKG1heENvbHMpOwogICAgIEFTU0VSVChzZWxmTmVlZHNM YXlvdXQoKSk7CiAKICAgICBtX25lZWRzU2VjdGlvblJlY2FsYyA9IGZhbHNlOwpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRhYmxlLmggYi9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyVGFibGUuaAppbmRleCBmOWY5Mzg2N2YyZTExNDEzNmI2MTc4MDk2 YWFhNzg5NzcyNTBiZGQxLi4zNzU0OGZjMzA2NmE4YjVlNzI2MzgwZjU2OWFmYjMxMjBiOGI2ODRh IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGUuaAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGUuaApAQCAtMzA1LDYgKzMwNSw4 IEBAIHByaXZhdGU6CiAKICAgICB2b2lkIGRpc3RyaWJ1dGVFeHRyYUxvZ2ljYWxIZWlnaHQoaW50 IGV4dHJhTG9naWNhbEhlaWdodCk7CiAKKyAgICB2b2lkIHJlc2l6ZUNvbHVtbnModW5zaWduZWQg bmV3U2l6ZSkgY29uc3Q7CisKICAgICBtdXRhYmxlIFZlY3RvcjxpbnQ+IG1fY29sdW1uUG9zOwog ICAgIG11dGFibGUgVmVjdG9yPENvbHVtblN0cnVjdD4gbV9jb2x1bW5zOwogICAgIG11dGFibGUg VmVjdG9yPFJlbmRlclRhYmxlQ2FwdGlvbio+IG1fY2FwdGlvbnM7Cg==