94522 2012-08-20 13:37:27 -0700 Unsafe vsprintf usage in TestNetscapePlugin 2012-08-20 16:39:58 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 japhet japhet abarth tony tsepez webkit.review.bot oldest_to_newest 699879 0 japhet 2012-08-20 13:37:27 -0700 Original report: http://code.google.com/p/chromium/issues/detail?id=141806 This is test-only code, so not marking it as a security issue. However, it is triggering crashes in ClusterFuzz, so it seems worth it to clean up. Using vsnprintf and a proper buffer size seems to fix the problem. 699884 1 159511 japhet 2012-08-20 13:41:55 -0700 Created attachment 159511 patch 699894 2 159511 abarth 2012-08-20 13:47:30 -0700 Comment on attachment 159511 patch View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66 > char message[2048] = "PLUGIN: "; > - vsprintf(message + strlen(message), format, args); > + vsnprintf(message + strlen(message), 2040, format, args); Why 2040 and not 2048 ? Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions. 699898 3 japhet 2012-08-20 13:49:04 -0700 (In reply to comment #2) > (From update of attachment 159511 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review > > > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66 > > char message[2048] = "PLUGIN: "; > > - vsprintf(message + strlen(message), format, args); > > + vsnprintf(message + strlen(message), 2040, format, args); > > Why 2040 and not 2048 ? Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions. Because the first 8 bytes are the "PLUGIN: ", and I'm a terrible person who uses magic numbers. Any preference on how I make this clearer? Good point on the '\0'. 699902 4 159511 tsepez 2012-08-20 13:51:02 -0700 Comment on attachment 159511 patch View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review >> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66 >> + vsnprintf(message + strlen(message), 2040, format, args); > > Why 2040 and not 2048 ? Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions. Also, so long as your calling strlen(), seems like both of these values should derive from that - as in 2048 - strlen(message). But don't call strlen() twice. sizeof("PLUGIN: ") -1 is also a suitable alternate. 699905 5 159511 abarth 2012-08-20 13:51:50 -0700 Comment on attachment 159511 patch View in context: https://bugs.webkit.org/attachment.cgi?id=159511&action=review >>> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:66 >>> + vsnprintf(message + strlen(message), 2040, format, args); >> >> Why 2040 and not 2048 ? Also, these libc functions don't necessarily \0-terminate strings, so it's a good practice to explicitly write a '\0' at the end of the array after calling these functions. > > Because the first 8 bytes are the "PLUGIN: ", and I'm a terrible person who uses magic numbers. Any preference on how I make this clearer? > > Good point on the '\0'. I see. Yeah, that's easy to for dumb people like me to miss. :) const size_t messageBufferSize = 2048; char message[2048] = "PLUGIN: "; messageLength = strlen(message); vsnprintf(message + messageLength, messageBufferSize - messageLength, format, args); message[messageBufferSize - 1] = '\0'; 699913 6 tsepez 2012-08-20 13:57:24 -0700 > char message[2048] = "PLUGIN: "; > messageLength = sizeof("PLUGIN: ") - 1; > vsnprintf(message + messageLength, sizeof(message) - messageLength, format, args); > message[sizeof(message) - 1] = '\0'; 699920 7 abarth 2012-08-20 14:02:44 -0700 tsepez puts me to shame, as usual :) 699944 8 tsepez 2012-08-20 14:13:49 -0700 Still not happy that we say "Plugin" twice. You should be able to tweak that string or the dimension of the buffer in one place and only one place and still have the code work. Let's see: > char message[2048]; > static const char plugin[] = "PLUGIN: "; > memcpy(message, plugin, sizeof(plugin) - 1); > vsnprintf(message + sizeof(plugin) - 1, sizeof(message) - (sizeof(plugin) - 1), format, args); > message[sizeof(message) - 1] = '\0'; But I've not tried it. 699958 9 159518 japhet 2012-08-20 14:21:34 -0700 Created attachment 159518 patch 699979 10 159518 abarth 2012-08-20 14:32:46 -0700 Comment on attachment 159518 patch This isn't my favorite idiom, but whatever man. 699986 11 japhet 2012-08-20 14:35:32 -0700 (In reply to comment #10) > (From update of attachment 159518 [details]) > This isn't my favorite idiom, but whatever man. The way I wrote the code, or any code that matches .*printf.* ? :) 699991 12 abarth 2012-08-20 14:38:20 -0700 > The way I wrote the code, or any code that matches .*printf.* ? :) :) 699993 13 abarth 2012-08-20 14:39:16 -0700 I think the root of the issue is that .*printf.* are so impossible to use correctly that they make the code that calls them super ugly, regardless of how it's written. 700162 14 159518 webkit.review.bot 2012-08-20 16:39:54 -0700 Comment on attachment 159518 patch Clearing flags on attachment: 159518 Committed r126089: <http://trac.webkit.org/changeset/126089> 700163 15 webkit.review.bot 2012-08-20 16:39:58 -0700 All reviewed patches have been landed. Closing bug. 159511 2012-08-20 13:41:55 -0700 2012-08-20 14:21:34 -0700 patch vsprintf.txt text/plain 2695 japhet SW5kZXg6IFRvb2xzL0R1bXBSZW5kZXJUcmVlL1Rlc3ROZXRzY2FwZVBsdWdJbi9QbHVnaW5PYmpl Y3QuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL0R1bXBSZW5kZXJUcmVlL1Rlc3ROZXRzY2FwZVBs dWdJbi9QbHVnaW5PYmplY3QuY3BwCShyZXZpc2lvbiAxMjU5MzUpCisrKyBUb29scy9EdW1wUmVu ZGVyVHJlZS9UZXN0TmV0c2NhcGVQbHVnSW4vUGx1Z2luT2JqZWN0LmNwcAkod29ya2luZyBjb3B5 KQpAQCAtNjAsMjMgKzYwLDEwIEBACiAgICAgYnJvd3Nlci0+cmVsZWFzZW9iamVjdChjb25zb2xl T2JqZWN0KTsKIH0KIAotLy8gSGVscGVyIGZ1bmN0aW9uIHdoaWNoIHRha2VzIGluIHRoZSBwbHVn aW4gd2luZG93IG9iamVjdCBmb3IgbG9nZ2luZyB0byB0aGUgY29uc29sZSBvYmplY3QuIFRoaXMg ZnVuY3Rpb24gc3VwcG9ydHMgdmFyaWFibGUKLS8vIGFyZ3VtZW50cy4KLXN0YXRpYyB2b2lkIHBs dWdpbkxvZ1dpdGhXaW5kb3dPYmplY3RWYXJpYWJsZUFyZ3MoTlBPYmplY3QqIHdpbmRvd09iamVj dCwgTlBQIGluc3RhbmNlLCBjb25zdCBjaGFyKiBmb3JtYXQsIC4uLikKLXsKLSAgICB2YV9saXN0 IGFyZ3M7Ci0gICAgdmFfc3RhcnQoYXJncywgZm9ybWF0KTsKLSAgICBjaGFyIG1lc3NhZ2VbMjA0 OF0gPSAiUExVR0lOOiAiOwotICAgIHZzcHJpbnRmKG1lc3NhZ2UgKyBzdHJsZW4obWVzc2FnZSks IGZvcm1hdCwgYXJncyk7Ci0gICAgdmFfZW5kKGFyZ3MpOwotCi0gICAgcGx1Z2luTG9nV2l0aFdp bmRvd09iamVjdCh3aW5kb3dPYmplY3QsIGluc3RhbmNlLCBtZXNzYWdlKTsKLX0KLSAgICAgICAg ICAgICAKIHZvaWQgcGx1Z2luTG9nV2l0aEFyZ3VtZW50cyhOUFAgaW5zdGFuY2UsIGNvbnN0IGNo YXIqIGZvcm1hdCwgdmFfbGlzdCBhcmdzKQogewogICAgIGNoYXIgbWVzc2FnZVsyMDQ4XSA9ICJQ TFVHSU46ICI7Ci0gICAgdnNwcmludGYobWVzc2FnZSArIHN0cmxlbihtZXNzYWdlKSwgZm9ybWF0 LCBhcmdzKTsKKyAgICB2c25wcmludGYobWVzc2FnZSArIHN0cmxlbihtZXNzYWdlKSwgMjA0MCwg Zm9ybWF0LCBhcmdzKTsKIAogICAgIE5QT2JqZWN0KiB3aW5kb3dPYmplY3QgPSAwOwogICAgIE5Q RXJyb3IgZXJyb3IgPSBicm93c2VyLT5nZXR2YWx1ZShpbnN0YW5jZSwgTlBOVldpbmRvd05QT2Jq ZWN0LCAmd2luZG93T2JqZWN0KTsKQEAgLTkzNiw3ICs5MjMsNyBAQAogICAgICAgICByZXR1cm4g ZmFsc2U7CiAgICAgfQogCi0gICAgcGx1Z2luTG9nV2l0aFdpbmRvd09iamVjdFZhcmlhYmxlQXJn cyh3aW5kb3dPYmplY3QsIG5wcCwgIkRPQ1VNRU5UIE9QRU4gU1VDQ0VTUyIpOworICAgIHBsdWdp bkxvZ1dpdGhXaW5kb3dPYmplY3Qod2luZG93T2JqZWN0LCBucHAsICJQTFVHSU46IERPQ1VNRU5U IE9QRU4gU1VDQ0VTUyIpOwogICAgIG5vdGlmeVRlc3RDb21wbGV0aW9uKG5wcCwgcmVzdWx0LnZh bHVlLm9iamVjdFZhbHVlKTsKICAgICBicm93c2VyLT5yZWxlYXNlb2JqZWN0KHJlc3VsdC52YWx1 ZS5vYmplY3RWYWx1ZSk7CiAgICAgYnJvd3Nlci0+cmVsZWFzZW9iamVjdCh3aW5kb3dPYmplY3Qp OwpAQCAtOTY4LDcgKzk1NSw3IEBACiAgICAgICAgIHJldHVybiBmYWxzZTsKICAgICB9CiAKLSAg ICBwbHVnaW5Mb2dXaXRoV2luZG93T2JqZWN0VmFyaWFibGVBcmdzKHdpbmRvd09iamVjdCwgbnBw LCAiV0lORE9XIE9QRU4gU1VDQ0VTUyIpOworICAgIHBsdWdpbkxvZ1dpdGhXaW5kb3dPYmplY3Qo d2luZG93T2JqZWN0LCBucHAsICJQTFVHSU46IFdJTkRPVyBPUEVOIFNVQ0NFU1MiKTsKICAgICBu b3RpZnlUZXN0Q29tcGxldGlvbihucHAsIHJlc3VsdC52YWx1ZS5vYmplY3RWYWx1ZSk7CiAgICAg YnJvd3Nlci0+cmVsZWFzZW9iamVjdChyZXN1bHQudmFsdWUub2JqZWN0VmFsdWUpOwogICAgIGJy b3dzZXItPnJlbGVhc2VvYmplY3Qod2luZG93T2JqZWN0KTsKSW5kZXg6IFRvb2xzL0NoYW5nZUxv Zwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyNjA1OSkKKysrIFRv b2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA4LTIw ICBOYXRlIENoYXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgVW5zYWZlIHZz cHJpbnRmIHVzYWdlIGluIFRlc3ROZXRzY2FwZVBsdWdpbgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTQ1MjIKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIER1bXBSZW5kZXJUcmVlL1Rlc3ROZXRzY2FwZVBs dWdJbi9QbHVnaW5PYmplY3QuY3BwOgorICAgICAgICAocGx1Z2luTG9nV2l0aEFyZ3VtZW50cyk6 IFVzaW5nIHZzbnByaW50ZiBpbnN0ZWFkIG9mIHZzcHJpbnRmIHRvIGVuc3VyZSB3ZSBkb24ndCBv dmVyZmxvdworICAgICAgICAgICAgdGhlIG1lc3NhZ2UgYnVmZmVyLgorICAgICAgICAodGVzdERv Y3VtZW50T3Blbik6CisgICAgICAgICh0ZXN0V2luZG93T3Blbik6CisKIDIwMTItMDgtMjAgIERp cmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgTlJXVCByZXBvcnRz IHVuZXhwZWN0ZWQgRU9GCg== 159518 2012-08-20 14:21:34 -0700 2012-08-20 16:39:54 -0700 patch vsprintf.txt text/plain 2923 japhet SW5kZXg6IFRvb2xzL0R1bXBSZW5kZXJUcmVlL1Rlc3ROZXRzY2FwZVBsdWdJbi9QbHVnaW5PYmpl Y3QuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL0R1bXBSZW5kZXJUcmVlL1Rlc3ROZXRzY2FwZVBs dWdJbi9QbHVnaW5PYmplY3QuY3BwCShyZXZpc2lvbiAxMjU5MzUpCisrKyBUb29scy9EdW1wUmVu ZGVyVHJlZS9UZXN0TmV0c2NhcGVQbHVnSW4vUGx1Z2luT2JqZWN0LmNwcAkod29ya2luZyBjb3B5 KQpAQCAtNjAsMjMgKzYwLDEzIEBACiAgICAgYnJvd3Nlci0+cmVsZWFzZW9iamVjdChjb25zb2xl T2JqZWN0KTsKIH0KIAotLy8gSGVscGVyIGZ1bmN0aW9uIHdoaWNoIHRha2VzIGluIHRoZSBwbHVn aW4gd2luZG93IG9iamVjdCBmb3IgbG9nZ2luZyB0byB0aGUgY29uc29sZSBvYmplY3QuIFRoaXMg ZnVuY3Rpb24gc3VwcG9ydHMgdmFyaWFibGUKLS8vIGFyZ3VtZW50cy4KLXN0YXRpYyB2b2lkIHBs dWdpbkxvZ1dpdGhXaW5kb3dPYmplY3RWYXJpYWJsZUFyZ3MoTlBPYmplY3QqIHdpbmRvd09iamVj dCwgTlBQIGluc3RhbmNlLCBjb25zdCBjaGFyKiBmb3JtYXQsIC4uLikKLXsKLSAgICB2YV9saXN0 IGFyZ3M7Ci0gICAgdmFfc3RhcnQoYXJncywgZm9ybWF0KTsKLSAgICBjaGFyIG1lc3NhZ2VbMjA0 OF0gPSAiUExVR0lOOiAiOwotICAgIHZzcHJpbnRmKG1lc3NhZ2UgKyBzdHJsZW4obWVzc2FnZSks IGZvcm1hdCwgYXJncyk7Ci0gICAgdmFfZW5kKGFyZ3MpOwotCi0gICAgcGx1Z2luTG9nV2l0aFdp bmRvd09iamVjdCh3aW5kb3dPYmplY3QsIGluc3RhbmNlLCBtZXNzYWdlKTsKLX0KLSAgICAgICAg ICAgICAKIHZvaWQgcGx1Z2luTG9nV2l0aEFyZ3VtZW50cyhOUFAgaW5zdGFuY2UsIGNvbnN0IGNo YXIqIGZvcm1hdCwgdmFfbGlzdCBhcmdzKQogewotICAgIGNoYXIgbWVzc2FnZVsyMDQ4XSA9ICJQ TFVHSU46ICI7Ci0gICAgdnNwcmludGYobWVzc2FnZSArIHN0cmxlbihtZXNzYWdlKSwgZm9ybWF0 LCBhcmdzKTsKKyAgICBjb25zdCBzaXplX3QgbWVzc2FnZUJ1ZmZlclNpemUgPSAyMDQ4OworICAg IGNoYXIgbWVzc2FnZVttZXNzYWdlQnVmZmVyU2l6ZV0gPSAiUExVR0lOOiAiOworICAgIGludCBt ZXNzYWdlTGVuZ3RoID0gc2l6ZW9mKCJQTFVHSU46ICIpIC0gMTsKKyAgICBtZXNzYWdlTGVuZ3Ro ICs9IHZzbnByaW50ZihtZXNzYWdlICsgbWVzc2FnZUxlbmd0aCwgbWVzc2FnZUJ1ZmZlclNpemUg LSAxIC0gbWVzc2FnZUxlbmd0aCwgZm9ybWF0LCBhcmdzKTsKKyAgICBtZXNzYWdlW21lc3NhZ2VM ZW5ndGhdID0gJ1wwJzsKIAogICAgIE5QT2JqZWN0KiB3aW5kb3dPYmplY3QgPSAwOwogICAgIE5Q RXJyb3IgZXJyb3IgPSBicm93c2VyLT5nZXR2YWx1ZShpbnN0YW5jZSwgTlBOVldpbmRvd05QT2Jq ZWN0LCAmd2luZG93T2JqZWN0KTsKQEAgLTkzNiw3ICs5MjYsNyBAQAogICAgICAgICByZXR1cm4g ZmFsc2U7CiAgICAgfQogCi0gICAgcGx1Z2luTG9nV2l0aFdpbmRvd09iamVjdFZhcmlhYmxlQXJn cyh3aW5kb3dPYmplY3QsIG5wcCwgIkRPQ1VNRU5UIE9QRU4gU1VDQ0VTUyIpOworICAgIHBsdWdp bkxvZ1dpdGhXaW5kb3dPYmplY3Qod2luZG93T2JqZWN0LCBucHAsICJQTFVHSU46IERPQ1VNRU5U IE9QRU4gU1VDQ0VTUyIpOwogICAgIG5vdGlmeVRlc3RDb21wbGV0aW9uKG5wcCwgcmVzdWx0LnZh bHVlLm9iamVjdFZhbHVlKTsKICAgICBicm93c2VyLT5yZWxlYXNlb2JqZWN0KHJlc3VsdC52YWx1 ZS5vYmplY3RWYWx1ZSk7CiAgICAgYnJvd3Nlci0+cmVsZWFzZW9iamVjdCh3aW5kb3dPYmplY3Qp OwpAQCAtOTY4LDcgKzk1OCw3IEBACiAgICAgICAgIHJldHVybiBmYWxzZTsKICAgICB9CiAKLSAg ICBwbHVnaW5Mb2dXaXRoV2luZG93T2JqZWN0VmFyaWFibGVBcmdzKHdpbmRvd09iamVjdCwgbnBw LCAiV0lORE9XIE9QRU4gU1VDQ0VTUyIpOworICAgIHBsdWdpbkxvZ1dpdGhXaW5kb3dPYmplY3Qo d2luZG93T2JqZWN0LCBucHAsICJQTFVHSU46IFdJTkRPVyBPUEVOIFNVQ0NFU1MiKTsKICAgICBu b3RpZnlUZXN0Q29tcGxldGlvbihucHAsIHJlc3VsdC52YWx1ZS5vYmplY3RWYWx1ZSk7CiAgICAg YnJvd3Nlci0+cmVsZWFzZW9iamVjdChyZXN1bHQudmFsdWUub2JqZWN0VmFsdWUpOwogICAgIGJy b3dzZXItPnJlbGVhc2VvYmplY3Qod2luZG93T2JqZWN0KTsKSW5kZXg6IFRvb2xzL0NoYW5nZUxv Zwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyNjA1OSkKKysrIFRv b2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA4LTIw ICBOYXRlIENoYXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgVW5zYWZlIHZz cHJpbnRmIHVzYWdlIGluIFRlc3ROZXRzY2FwZVBsdWdpbgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTQ1MjIKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIER1bXBSZW5kZXJUcmVlL1Rlc3ROZXRzY2FwZVBs dWdJbi9QbHVnaW5PYmplY3QuY3BwOgorICAgICAgICAocGx1Z2luTG9nV2l0aEFyZ3VtZW50cyk6 IFVzaW5nIHZzbnByaW50ZiBpbnN0ZWFkIG9mIHZzcHJpbnRmIHRvIGVuc3VyZSB3ZSBkb24ndCBv dmVyZmxvdworICAgICAgICAgICAgdGhlIG1lc3NhZ2UgYnVmZmVyLgorICAgICAgICAodGVzdERv Y3VtZW50T3Blbik6CisgICAgICAgICh0ZXN0V2luZG93T3Blbik6CisKIDIwMTItMDgtMjAgIERp cmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgTlJXVCByZXBvcnRz IHVuZXhwZWN0ZWQgRU9GCg==