189070 2018-08-28 17:03:45 -0700 lldb-webkit: KeyError thrown for OptionSet with invalid value 2018-08-31 09:45:48 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 dbates dbates darin lforschler simon.fraser webkit-bug-importer oldest_to_newest 1454630 0 dbates 2018-08-28 17:03:45 -0700 An OptionSet variable can be in scope and uninitialized as a variable is not initialized until execution has flowed over its assignment. We need to take care to handle this case and not try to compute the set of enumerators in an uninitialized OptionSet. 1454638 1 348363 dbates 2018-08-28 17:17:32 -0700 Created attachment 348363 Patch 1454640 2 348363 dbates 2018-08-28 17:22:24 -0700 Comment on attachment 348363 Patch This is wrong. 1454947 3 348436 dbates 2018-08-29 15:12:02 -0700 Created attachment 348436 Patch 1454951 4 dbates 2018-08-29 15:20:28 -0700 Committed r235482: <https://trac.webkit.org/changeset/235482> 1454952 5 webkit-bug-importer 2018-08-29 15:21:17 -0700 <rdar://problem/43860433> 1455086 6 348436 darin 2018-08-29 18:13:33 -0700 Comment on attachment 348436 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348436&action=review > Tools/lldb/lldb_webkit.py:627 > + return # self.valobj is uninitialized memory This comment and the bug title is too specific and potentially incorrect. This code path can occur any time the OptionSet has an invalid value. An uninitialized variable is one case when something might have an invalid value, but of course something uninitialized might also have a valid value. But there are many others such as a variable that has been overwritten by a buffer overrun, a variable been overwritten by something writing through a stale pointer, trying to display a value after a local variable memory region is reused for another purpose because we are past the point of use of the local variable in the code, a value from following a pointer to an object that has already been deleted, and many more. On the other hand, I suspect the *behavior* of the code you wrote is probably good; it writes out something that lets you see the value of the memory and makes it clear that the OptionSet has an invalid value. 1455248 7 dbates 2018-08-30 09:40:51 -0700 (In reply to Darin Adler from comment #6) > Comment on attachment 348436 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348436&action=review > > > Tools/lldb/lldb_webkit.py:627 > > + return # self.valobj is uninitialized memory > > This comment and the bug title is too specific and potentially incorrect. > > This code path can occur any time the OptionSet has an invalid value. > > An uninitialized variable is one case when something might have an invalid > value, but of course something uninitialized might also have a valid value. > But there are many others such as a variable that has been overwritten by a > buffer overrun, a variable been overwritten by something writing through a > stale pointer, trying to display a value after a local variable memory > region is reused for another purpose because we are past the point of use of > the local variable in the code, a value from following a pointer to an > object that has already been deleted, and many more. You're right! We could either remove the comment or I could update the comment to explain that self.valobj has an invalid value, maybe: # self.valobj has an invalid value. What do you suggest? 1455499 8 darin 2018-08-30 18:25:44 -0700 Either removing the comment or updating it is fine with me. 1455500 9 darin 2018-08-30 18:26:34 -0700 An even more useful comment would make it clear why just returning early is the right thing to do for an invalid value. # Since this is an invalid value, return so the raw hex form is written out. Or a comment that is correct. 1455719 10 dbates 2018-08-31 09:45:48 -0700 (In reply to Darin Adler from comment #9) > An even more useful comment would make it clear why just returning early is > the right thing to do for an invalid value. > > # Since this is an invalid value, return so the raw hex form is written > out. > > Or a comment that is correct. Updated comment as well as the change log entry for r235482 to reflect the updated title for this bug and better describe the purpose of the change. Committed this in <https://trac.webkit.org/changeset/235555>. 348363 2018-08-28 17:17:32 -0700 2018-08-29 15:12:01 -0700 Patch bug-189070-20180828171731.patch text/plain 2739 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjM1MzgxCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMjYyYmE4YjdjNTI4ZDg3ZmZiOWU3MGY5ZDJlZWRjZGY2 ZWU3ZGU5My4uZDVmZDhjZGUzOGNmYWY3YjlkMmE1NTE1ODFlYjU5N2NiMDI5ZTM2OSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIy IEBACisyMDE4LTA4LTI4ICBEYW5pZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KKworICAg ICAgICBsbGRiLXdlYmtpdDogS2V5RXJyb3IgdGhyb3duIGZvciB1bmluaXRpYWxpemVkIE9wdGlv blNldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg5 MDcwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRG8g bm90IGNvbXB1dGUgd2hhdCBlbnVtZXJhdG9ycyBhcmUgaW4gYW4gdW5pbml0aWFsaXplZCBPcHRp b25TZXQuIEEgbG9jYWwgT3B0aW9uU2V0IHZhcmlhYmxlCisgICAgICAgIGlzIG9ubHkgY29uc2lk ZXJlZCBpbml0aWFsaXplZCB3aGVuIGV4ZWN1dGlvbiBwYXNzZXMgb3ZlciBpdHMgYXNzaWdubWVu dCByZWdhcmRsZXNzIG9mIHdoZXRoZXIKKyAgICAgICAgdGhlIHZhcmlhYmxlIGlzIGluIHNjb3Bl LgorCisgICAgICAgIFRoZSBMTERCIFB5dGhvbiBBUEkgZG9lcyBub3QgcHJvdmlkZSBhIHdheSB0 byBkZXRlcm1pbmUgd2hldGhlciBhbiB2YXJpYWJsZSBpcyBpbml0aWFsaXplZC4KKyAgICAgICAg U28sIHdlIHVzZSBhIHNpbXBsZSBoZXVyaXN0aWM6IHdoZW4gdGhlIHZhbHVlIG9mIHRoZSBPcHRp b25TZXQgaXMgZ3JlYXRlciB0aGFuIHRoYW4gdGhlIHZhbHVlCisgICAgICAgIG9mIHRoZSBsYXJn ZXN0IGVudW1lcmF0b3IgdGhlbiB3ZSBjb25zaWRlciB0aGUgT3B0aW9uU2V0IHRvIGJlIGdhcmJh Z2UgKHVuaW5pdGlhbGl6ZWQgbWVtb3J5KS4KKyAgICAgICAgV2hlbiB0aGUgdmFyaWFibGUgaXMg ZmluYWxseSBpbml0aWFsaXplZCBMTERCIHdpbGwgbm90aWZ5IHVzIHRvIHVwZGF0ZSBvdXIgc3Rh dGUuCisKKyAgICAgICAgKiBsbGRiL2xsZGJfd2Via2l0LnB5OgorICAgICAgICAoV1RGT3B0aW9u U2V0UHJvdmlkZXIudXBkYXRlKToKKwogMjAxOC0wOC0yOCAgRGFuaWVsIEJhdGVzICA8ZGFiYXRl c0BhcHBsZS5jb20+CiAKICAgICAgICAgbGxkYi13ZWJraXQ6IEtleUVycm9yIHRocm93biBmb3Ig T3B0aW9uU2V0CmRpZmYgLS1naXQgYS9Ub29scy9sbGRiL2xsZGJfd2Via2l0LnB5IGIvVG9vbHMv bGxkYi9sbGRiX3dlYmtpdC5weQppbmRleCA0NGQ3N2JiNWE3NmI5MzBjNWRkYzc4NmJjODlmNTdh YWRmMGYwZDA2Li5hYzZiYjMyZTlkZTlmZTEyMTEyMDBmMjFmZmVjZTI5OThiMDFlY2YzIDEwMDY0 NAotLS0gYS9Ub29scy9sbGRiL2xsZGJfd2Via2l0LnB5CisrKyBiL1Rvb2xzL2xsZGIvbGxkYl93 ZWJraXQucHkKQEAgLTYxMiwxNiArNjEyLDIyIEBAIGNsYXNzIFdURk9wdGlvblNldFByb3ZpZGVy OgogICAgICAgICByZXR1cm4gTm9uZQogCiAgICAgZGVmIHVwZGF0ZShzZWxmKToKLSAgICAgICAg c2VsZi5zdG9yYWdlID0gc2VsZi52YWxvYmouR2V0Q2hpbGRNZW1iZXJXaXRoTmFtZSgnbV9zdG9y YWdlJykKKyAgICAgICAgc2VsZi5zdG9yYWdlID0gc2VsZi52YWxvYmouR2V0Q2hpbGRNZW1iZXJX aXRoTmFtZSgnbV9zdG9yYWdlJykgICMgTWF5IGJlIHVuaW5pdGlhbGl6ZWQgbWVtb3J5CisgICAg ICAgIHNlbGYuX2VsZW1lbnRzID0gW10KKyAgICAgICAgc2VsZi5zaXplID0gMAogCiAgICAgICAg IHRlbXBsYXRlX2FyZ3VtZW50X3NiVHlwZSA9IHNlbGYudmFsb2JqLkdldFR5cGUoKS5HZXRUZW1w bGF0ZUFyZ3VtZW50VHlwZSgwKQogICAgICAgICBlbnVtZXJhdG9yX3ZhbHVlX3RvX25hbWVfbWFw ID0ge3NiVHlwZUVudW1NZW1iZXIuR2V0VmFsdWVBc1Vuc2lnbmVkKCk6IHNiVHlwZUVudW1NZW1i ZXIuR2V0TmFtZSgpIGZvciBzYlR5cGVFbnVtTWVtYmVyIGluIHRlbXBsYXRlX2FyZ3VtZW50X3Ni VHlwZS5nZXRfZW51bV9tZW1iZXJzX2FycmF5KCl9CiAgICAgICAgIGlmIG5vdCBlbnVtZXJhdG9y X3ZhbHVlX3RvX25hbWVfbWFwOgogICAgICAgICAgICAgcmV0dXJuCiAKKyAgICAgICAgYml0bWFz ayA9IHNlbGYuc3RvcmFnZS5HZXRWYWx1ZUFzVW5zaWduZWQoMCkKKyAgICAgICAgaWYgYml0bWFz ayA+IG1heChlbnVtZXJhdG9yX3ZhbHVlX3RvX25hbWVfbWFwKToKKyAgICAgICAgICAgIHJldHVy biAgIyBzZWxmLnZhbG9iaiBpcyB1bmluaXRpYWxpemVkIG1lbW9yeQorCisgICAgICAgICMgc2Vs Zi52YWxvYmogbG9va3MgbGlrZSBpdCBjb250YWlucyBhIHZhbGlkIHZhbHVlLgogICAgICAgICAj IEl0ZXJhdGUgZnJvbSBsZWFzdCBzaWduaWZpY2FudCBiaXQgdG8gbW9zdCBzaWduaWZpY2FudCBi aXQuCiAgICAgICAgIGVsZW1lbnRzID0gW10KLSAgICAgICAgYml0bWFzayA9IHNlbGYuc3RvcmFn ZS5HZXRWYWx1ZUFzVW5zaWduZWQoMCkKICAgICAgICAgd2hpbGUgYml0bWFzayA+IDA6CiAgICAg ICAgICAgICBjdXJyZW50ID0gYml0bWFzayAmIC1iaXRtYXNrICAjIElzb2xhdGUgdGhlIHJpZ2h0 bW9zdCBzZXQgYml0LgogICAgICAgICAgICAgZWxlbWVudHMuYXBwZW5kKChlbnVtZXJhdG9yX3Zh bHVlX3RvX25hbWVfbWFwW2N1cnJlbnRdLCBjdXJyZW50KSkgICMgZS5nLiAoJ1NwZWxsaW5nJywg NCkK 348436 2018-08-29 15:12:02 -0700 2018-08-29 15:13:05 -0700 Patch bug-189070-20180829151201.patch text/plain 2855 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjM1NDgxCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggODU2N2FhNGJlNmM2ZmIxNjU3MThiNzlkNGE1NzY2MDkw OWY3MWU2Zi4uMDU0YTlhOTY5NThjZTkwZWI2Mjc2MTU0Nzg0OGM1MmY5N2ZkMTY2OSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIz IEBACisyMDE4LTA4LTI5ICBEYW5pZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KKworICAg ICAgICBsbGRiLXdlYmtpdDogS2V5RXJyb3IgdGhyb3duIGZvciB1bmluaXRpYWxpemVkIE9wdGlv blNldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg5 MDcwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRG8g bm90IGNvbXB1dGUgd2hhdCBlbnVtZXJhdG9ycyBhcmUgaW4gYW4gdW5pbml0aWFsaXplZCBPcHRp b25TZXQuIEEgbG9jYWwgT3B0aW9uU2V0IHZhcmlhYmxlCisgICAgICAgIGlzIG9ubHkgY29uc2lk ZXJlZCBpbml0aWFsaXplZCB3aGVuIGV4ZWN1dGlvbiBwYXNzZXMgb3ZlciBpdHMgYXNzaWdubWVu dCByZWdhcmRsZXNzIG9mIHdoZXRoZXIKKyAgICAgICAgdGhlIHZhcmlhYmxlIGlzIGluIHNjb3Bl LgorCisgICAgICAgIFRoZSBMTERCIFB5dGhvbiBBUEkgZG9lcyBub3QgcHJvdmlkZSBhIHdheSB0 byBkZXRlcm1pbmUgd2hldGhlciBhbiB2YXJpYWJsZSBpcyBpbml0aWFsaXplZC4KKyAgICAgICAg U28sIHdlIHVzZSBhIHNpbXBsZSBoZXVyaXN0aWM6IHdoZW4gdGhlIHZhbHVlIG9mIHRoZSBPcHRp b25TZXQgaXMgZ3JlYXRlciB0aGFuIHRoZSB2YWx1ZQorICAgICAgICBvZiB0aGUgYml0bWFzayB3 aXRoIGFsbCBlbnVtZXJhdG9ycyBzZXQgdGhlbiB3ZSBjb25zaWRlciB0aGUgT3B0aW9uU2V0IHRv IGJlIGdhcmJhZ2UgKGkuZS4KKyAgICAgICAgdW5pbml0aWFsaXplZCBtZW1vcnkpLiBXaGVuIHRo ZSB2YXJpYWJsZSBpcyBmaW5hbGx5IGluaXRpYWxpemVkIExMREIgd2lsbCBub3RpZnkgdXMgdG8g dXBkYXRlCisgICAgICAgIG91ciBzdGF0ZS4KKworICAgICAgICAqIGxsZGIvbGxkYl93ZWJraXQu cHk6CisgICAgICAgIChXVEZPcHRpb25TZXRQcm92aWRlci51cGRhdGUpOgorCiAyMDE4LTA4LTI5 ICBDaHJpcyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CiAKICAgICAgICAgW1BTT05dIFdlIHNo b3VsZCBvbmx5IHByb2Nlc3Mtc3dhcCB3aGVuIGVUTEQrMSBjaGFuZ2VzIG9uIG5hdmlnYXRpb24K ZGlmZiAtLWdpdCBhL1Rvb2xzL2xsZGIvbGxkYl93ZWJraXQucHkgYi9Ub29scy9sbGRiL2xsZGJf d2Via2l0LnB5CmluZGV4IDQ0ZDc3YmI1YTc2YjkzMGM1ZGRjNzg2YmM4OWY1N2FhZGYwZjBkMDYu LmY0ZTFlMmI5Y2E4YTExMDg2OGJhMmE2MWJiMzc2Y2ZlY2VkYjE2OGIgMTAwNjQ0Ci0tLSBhL1Rv b2xzL2xsZGIvbGxkYl93ZWJraXQucHkKKysrIGIvVG9vbHMvbGxkYi9sbGRiX3dlYmtpdC5weQpA QCAtNjEyLDE2ICs2MTIsMjMgQEAgY2xhc3MgV1RGT3B0aW9uU2V0UHJvdmlkZXI6CiAgICAgICAg IHJldHVybiBOb25lCiAKICAgICBkZWYgdXBkYXRlKHNlbGYpOgotICAgICAgICBzZWxmLnN0b3Jh Z2UgPSBzZWxmLnZhbG9iai5HZXRDaGlsZE1lbWJlcldpdGhOYW1lKCdtX3N0b3JhZ2UnKQorICAg ICAgICBzZWxmLnN0b3JhZ2UgPSBzZWxmLnZhbG9iai5HZXRDaGlsZE1lbWJlcldpdGhOYW1lKCdt X3N0b3JhZ2UnKSAgIyBNYXkgYmUgdW5pbml0aWFsaXplZCBtZW1vcnkKKyAgICAgICAgc2VsZi5f ZWxlbWVudHMgPSBbXQorICAgICAgICBzZWxmLnNpemUgPSAwCiAKICAgICAgICAgdGVtcGxhdGVf YXJndW1lbnRfc2JUeXBlID0gc2VsZi52YWxvYmouR2V0VHlwZSgpLkdldFRlbXBsYXRlQXJndW1l bnRUeXBlKDApCiAgICAgICAgIGVudW1lcmF0b3JfdmFsdWVfdG9fbmFtZV9tYXAgPSB7c2JUeXBl RW51bU1lbWJlci5HZXRWYWx1ZUFzVW5zaWduZWQoKTogc2JUeXBlRW51bU1lbWJlci5HZXROYW1l KCkgZm9yIHNiVHlwZUVudW1NZW1iZXIgaW4gdGVtcGxhdGVfYXJndW1lbnRfc2JUeXBlLmdldF9l bnVtX21lbWJlcnNfYXJyYXkoKX0KICAgICAgICAgaWYgbm90IGVudW1lcmF0b3JfdmFsdWVfdG9f bmFtZV9tYXA6CiAgICAgICAgICAgICByZXR1cm4KIAorICAgICAgICBiaXRtYXNrX3dpdGhfYWxs X29wdGlvbnNfc2V0ID0gc3VtKGVudW1lcmF0b3JfdmFsdWVfdG9fbmFtZV9tYXApCisgICAgICAg IGJpdG1hc2sgPSBzZWxmLnN0b3JhZ2UuR2V0VmFsdWVBc1Vuc2lnbmVkKDApCisgICAgICAgIGlm IGJpdG1hc2sgPiBiaXRtYXNrX3dpdGhfYWxsX29wdGlvbnNfc2V0OgorICAgICAgICAgICAgcmV0 dXJuICAjIHNlbGYudmFsb2JqIGlzIHVuaW5pdGlhbGl6ZWQgbWVtb3J5CisKKyAgICAgICAgIyBz ZWxmLnZhbG9iaiBsb29rcyBsaWtlIGl0IGNvbnRhaW5zIGEgdmFsaWQgdmFsdWUuCiAgICAgICAg ICMgSXRlcmF0ZSBmcm9tIGxlYXN0IHNpZ25pZmljYW50IGJpdCB0byBtb3N0IHNpZ25pZmljYW50 IGJpdC4KICAgICAgICAgZWxlbWVudHMgPSBbXQotICAgICAgICBiaXRtYXNrID0gc2VsZi5zdG9y YWdlLkdldFZhbHVlQXNVbnNpZ25lZCgwKQogICAgICAgICB3aGlsZSBiaXRtYXNrID4gMDoKICAg ICAgICAgICAgIGN1cnJlbnQgPSBiaXRtYXNrICYgLWJpdG1hc2sgICMgSXNvbGF0ZSB0aGUgcmln aHRtb3N0IHNldCBiaXQuCiAgICAgICAgICAgICBlbGVtZW50cy5hcHBlbmQoKGVudW1lcmF0b3Jf dmFsdWVfdG9fbmFtZV9tYXBbY3VycmVudF0sIGN1cnJlbnQpKSAgIyBlLmcuICgnU3BlbGxpbmcn LCA0KQo=