hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andy Isaacson (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-8686) hadoop-common: fix warnings in native code
Date Wed, 15 Aug 2012 00:23:38 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-8686?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13434691#comment-13434691

Andy Isaacson commented on HADOOP-8686:

+void validate_size_t_is_bigger_than_jint(void) {
+  /* 
+   * size_t is between 2 bytes and 8 bytes, depending on platform.
+   * jint is always 4 bytes.
+   *
+   * If sizeof(size_t) is smaller than sizeof(jint), this declaration will
+   * attempt to create a negative array, which would be a compiler error.
+   * Effectively, this asserts at compile time that sizeof(size_t) is bigger than
+   * sizeof(jint).
+   */
+  unsigned char can_convert_jint_to_sizet[sizeof(size_t) - sizeof(jint)];
+  memset(&can_convert_jint_to_sizet, 0, sizeof(can_convert_jint_to_sizet));
While it's true there exist architectures where size_t is 2 bytes, none of them are likely
to run Hadoop. I think this should just be a documentation comment at most, and write the
code under the assumption that size_t is at least 32 bits.

-  snappy_status ret = dlsym_snappy_compress(uncompressed_bytes, uncompressed_direct_buf_len,
compressed_bytes, &compressed_direct_buf_len);
+  buf_len = (size_t)compressed_direct_buf_len; /* see validate_size_t_is_bigger_than_jint
+  snappy_status ret = dlsym_snappy_compress(uncompressed_bytes,
+        uncompressed_direct_buf_len, compressed_bytes, &buf_len);
This is fixing a stack smash via the third argument to snappy_compress.  This is a Must Fix
+  if ((buf_len > 0x7fffffffLLU) || (buf_len < 0)) {
Most gccs warn if you write {{unsigned < 0}}, so I suspect this is adding a warning since
size_t is unsigned 64 bit on amd64?

I think we'd be better off with {{if (buf_len > INT_MAX)}} or something along those lines.
 Either using the {{<limits.h>}} define or defining our own {{JINT_MAX}} would be good.

 static void throw_ioe(JNIEnv* env, int errnum)
+  if (errnum < 0) {
+    errnum = -errnum;
+  }
This "handle -errno" fixup does not belong in a common helper like throw_ioe.  The helper
should either take an errno value or the negation of an errno value, it shouldn't conflate
them like this. It's the caller's responsibility (together with ensuring that the library
as a whole has a consistent error description model) to ensure that the argument is consistently
in the correct range.

Also please add a documentation comment to describe the calling conventions for throw_ioe.

-    message = "Unknown error";
+    snprintf(message, sizeof(message), "unknown error %d", errnum);
I wouldn't mention it if this were the only issue, but since we need to fix the above: please
make the format start with a capital, "Unknown error %d".

The rest of the change LGTM.
> hadoop-common: fix warnings in native code
> ------------------------------------------
>                 Key: HADOOP-8686
>                 URL: https://issues.apache.org/jira/browse/HADOOP-8686
>             Project: Hadoop Common
>          Issue Type: Bug
>          Components: native
>    Affects Versions: 2.2.0-alpha
>            Reporter: Colin Patrick McCabe
>            Assignee: Colin Patrick McCabe
>            Priority: Minor
>         Attachments: HADOOP-8686.002.patch
> Fix warnings about const-correctness, improper conversion between pointers of different
sizes, implicit declaration of sync_file_range, variables being used with uninitialized values,
and so forth in the hadoop-common native code.

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


View raw message