/hg/icedtea-web: Changed usage of sprintf to snprintf. Made some...
Pavel Tisnovsky
ptisnovs at redhat.com
Wed May 23 02:23:27 PDT 2012
Hi Adam,
most changes looks ok - I mean the changes from malloced strings to a normal char arrays.
I just think it would be better to replace "magical" constants to a symbolic constants, ie.
20 // max = long long = 8446744073709551615 == 19 chars etc.
I'm not sure what's the actual meaning of following lines (either old and new version):
> - sprintf(parts[0], "");
> - sprintf(parts[1], "");
> - sprintf(parts[2], "");
> + snprintf(parts[0], sizeof(""), "");
> + snprintf(parts[1], sizeof(""), "");
> + snprintf(parts[2], sizeof(""), "");
The new version is not safer than the old version, because to made it safer you need
to set max.string length according to destination buffer, not according to source buffer.
After all is not it the same as pushing '\0' to the first byte of parts[*] array?
(it would be much faster)
Cheers,
Pavel
adomurad at icedtea.classpath.org wrote:
> changeset c367faabd08e in /hg/icedtea-web
> details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=c367faabd08e
> author: Adam Domurad <adomurad at redhat.com>
> date: Tue May 22 14:19:50 2012 -0400
>
> Changed usage of sprintf to snprintf. Made some small malloc'd buffers on the stack.
> These changes are unlikely to change functionality, for if the buffer is too small to write to, something has already gone wrong. However, they are good as an additional safety guarantee, preventing memory from corruption in the case that something goes wrong.
>
>
> diffstat:
>
> ChangeLog | 12 +++++++
> plugin/icedteanp/IcedTeaNPPlugin.cc | 6 +-
> plugin/icedteanp/IcedTeaPluginUtils.cc | 54 +++++++++++++++++----------------
> 3 files changed, 43 insertions(+), 29 deletions(-)
>
> diffs (169 lines):
>
> diff -r 7bbba99d25cc -r c367faabd08e ChangeLog
> --- a/ChangeLog Tue May 22 14:21:10 2012 +0200
> +++ b/ChangeLog Tue May 22 14:19:50 2012 -0400
> @@ -1,3 +1,15 @@
> +2012-05-22 Adam Domurad <adomurad at redhat.com>
> +
> + Changed allocation of small, fixed-size buffers to stack-based
> + allocations. Changed occurences of sprintf to the safer function
> + snprintf, added buffer information. While unlikely to change
> + functionality, snprintf adds an extra check to prevent buffer
> + overflows.
> + * plugin/icedteanp/IcedTeaNPPlugin.cc: Allocation of small buffers
> + using malloc changed to stack allocation & changed sprintf calls to
> + buffer-size aware snprintf calls.
> + * plugin/icedteanp/IcedTeaPluginUtils.cc: Same as above.
> +
> 2012-05-22 Jiri Vanek <jvanek at redhat.com>
>
> * tests/jnlp_tests/signed/ReadPropertiesSigned/testcases/ReadPropertiesSignedTest.java:
> diff -r 7bbba99d25cc -r c367faabd08e plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc Tue May 22 14:21:10 2012 +0200
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc Tue May 22 14:19:50 2012 -0400
> @@ -1227,9 +1227,9 @@
> {
>
> // clear the "instance X status" parts
> - sprintf(parts[0], "");
> - sprintf(parts[1], "");
> - sprintf(parts[2], "");
> + snprintf(parts[0], sizeof(""), "");
> + snprintf(parts[1], sizeof(""), "");
> + snprintf(parts[2], sizeof(""), "");
>
> // join the rest
> gchar* status_message = g_strjoinv(" ", parts);
> diff -r 7bbba99d25cc -r c367faabd08e plugin/icedteanp/IcedTeaPluginUtils.cc
> --- a/plugin/icedteanp/IcedTeaPluginUtils.cc Tue May 22 14:21:10 2012 +0200
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.cc Tue May 22 14:19:50 2012 -0400
> @@ -147,21 +147,20 @@
> IcedTeaPluginUtilities::JSIDToString(void* id, std::string* result)
> {
>
> - char* id_str = (char*) malloc(sizeof(char)*20); // max = long long = 8446744073709551615 == 19 chars
> + char id_str[20]; // max = long long = 8446744073709551615 == 19 chars
>
> if (sizeof(void*) == sizeof(long long))
> {
> - sprintf(id_str, "%llu", id);
> + snprintf(id_str, sizeof(id_str), "%llu", id);
> }
> else
> {
> - sprintf(id_str, "%lu", id); // else use long
> + snprintf(id_str, sizeof(id_str), "%lu", id); // else use long
> }
>
> result->append(id_str);
>
> PLUGIN_DEBUG("Converting pointer %p to %s\n", id, id_str);
> - free(id_str);
> }
>
> /**
> @@ -258,11 +257,9 @@
> IcedTeaPluginUtilities::itoa(int i, std::string* result)
> {
> // largest possible integer is 10 digits long
> - char* int_str = (char*) malloc(sizeof(char)*11);
> - sprintf(int_str, "%d", i);
> + char int_str[11];
> + snprintf(int_str, sizeof(int_str), "%d", i);
> result->append(int_str);
> -
> - free(int_str);
> }
>
> /**
> @@ -372,18 +369,17 @@
> ostream << length;
>
> // UTF-8 characters are 4-bytes max + space + '\0'
> - char* hex_value = (char*) malloc(sizeof(char)*10);
> + char hex_value[10];
>
> for (int i = 0; i < str->length(); i++)
> {
> - sprintf(hex_value, " %hx", str->at(i));
> + snprintf(hex_value, sizeof(hex_value)," %hx", str->at(i));
> ostream << hex_value;
> }
>
> utf_str->clear();
> *utf_str = ostream.str();
>
> - free(hex_value);
> PLUGIN_DEBUG("Converted %s to UTF-8 string %s\n", str->c_str(), utf_str->c_str());
> }
>
> @@ -683,49 +679,55 @@
> void
> IcedTeaPluginUtilities::NPVariantToString(NPVariant variant, std::string* result)
> {
> - char* str = (char*) malloc(sizeof(char)*32); // enough for everything except string
> + char str[32]; // enough for everything except string
> + char* largestr = NULL;
>
> if (NPVARIANT_IS_VOID(variant))
> {
> - sprintf(str, "%p", variant);
> + snprintf(str, sizeof(str), "%p", variant);
> }
> else if (NPVARIANT_IS_NULL(variant))
> {
> - sprintf(str, "NULL");
> + snprintf(str, sizeof(str), "NULL");
> }
> else if (NPVARIANT_IS_BOOLEAN(variant))
> {
> if (NPVARIANT_TO_BOOLEAN(variant))
> - sprintf(str, "true");
> + snprintf(str, sizeof(str), "true");
> else
> - sprintf(str, "false");
> + snprintf(str, sizeof(str), "false");
> }
> else if (NPVARIANT_IS_INT32(variant))
> {
> - sprintf(str, "%d", NPVARIANT_TO_INT32(variant));
> + snprintf(str, sizeof(str), "%d", NPVARIANT_TO_INT32(variant));
> }
> else if (NPVARIANT_IS_DOUBLE(variant))
> {
> - sprintf(str, "%f", NPVARIANT_TO_DOUBLE(variant));;
> + snprintf(str, sizeof(str), "%f", NPVARIANT_TO_DOUBLE(variant));;
> }
> else if (NPVARIANT_IS_STRING(variant))
> {
> free(str);
> #if MOZILLA_VERSION_COLLAPSED < 1090200
> - str = (char*) malloc(sizeof(char)*NPVARIANT_TO_STRING(variant).utf8length);
> - sprintf(str, "%s", NPVARIANT_TO_STRING(variant).utf8characters);
> + size_t buffersize = sizeof(char)*NPVARIANT_TO_STRING(variant).utf8length;
> + largestr = (char*) malloc(buffersize);
> + snprintf(str, buffersize, "%s", NPVARIANT_TO_STRING(variant).utf8characters);
> #else
> - str = (char*) malloc(sizeof(char)*NPVARIANT_TO_STRING(variant).UTF8Length);
> - sprintf(str, "%s", NPVARIANT_TO_STRING(variant).UTF8Characters);
> + size_t buffersize = sizeof(char)*NPVARIANT_TO_STRING(variant).UTF8Length;
> + largestr = (char*) malloc(buffersize);
> + snprintf(str, buffersize, "%s", NPVARIANT_TO_STRING(variant).UTF8Characters);
> #endif
> }
> else
> {
> - sprintf(str, "[Object %p]", variant);
> + snprintf(str, sizeof(str), "[Object %p]", variant);
> }
> -
> - result->append(str);
> - free(str);
> + if (largestr != NULL){
> + result->append(largestr);
> + free(largestr);
> + } else {
> + result->append(str);
> + }
> }
>
> bool
More information about the distro-pkg-dev
mailing list