Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: native
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      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.

      1. HADOOP-8686.005.patch
        10 kB
        Colin Patrick McCabe
      2. HADOOP-8686.002.patch
        10 kB
        Colin Patrick McCabe

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1173 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1173/)
        HADOOP-8686. Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301)

        Result = FAILURE
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1173 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1173/ ) HADOOP-8686 . Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1141 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1141/)
        HADOOP-8686. Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301)

        Result = FAILURE
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1141 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1141/ ) HADOOP-8686 . Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #2637 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2637/)
        HADOOP-8686. Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301)

        Result = FAILURE
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2637 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2637/ ) HADOOP-8686 . Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #2608 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2608/)
        HADOOP-8686. Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301)

        Result = SUCCESS
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2608 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2608/ ) HADOOP-8686 . Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #2672 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2672/)
        HADOOP-8686. Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301)

        Result = SUCCESS
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2672 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2672/ ) HADOOP-8686 . Fix warnings in native code. Contributed by Colin Patrick McCabe (Revision 1375301) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1375301 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Compressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/lz4/Lz4Decompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/compress/snappy/SnappyCompressor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsNetgroupMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/NativeCrc32.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/util/bulk_crc32.c
        Hide
        Eli Collins added a comment -

        I've committed this to trunk and merged to branch-2. Thanks Colin!

        Show
        Eli Collins added a comment - I've committed this to trunk and merged to branch-2. Thanks Colin!
        Hide
        Eli Collins added a comment -

        +1 looks great, didn't find anything Andy didn't catch.

        Show
        Eli Collins added a comment - +1 looks great, didn't find anything Andy didn't catch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12540992/HADOOP-8686.005.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1300//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1300//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12540992/HADOOP-8686.005.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1300//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1300//console This message is automatically generated.
        Hide
        Colin Patrick McCabe added a comment -

        correction: we actually tried to print the 64-bit number with %ld

        Show
        Colin Patrick McCabe added a comment - correction: we actually tried to print the 64-bit number with %ld
        Hide
        Colin Patrick McCabe added a comment -
        • capitalize "Unknown error"
        • make a comment about size_t needing to be bigger than 2 bytes
        • define JINT_MAX and use it
        • throw_ioe: don't treat negative values the same as positive ones
        • fix a case where we got compiler warnings on 32-bit compiles because we tried to print a 64-bit number with '%d'
        Show
        Colin Patrick McCabe added a comment - capitalize "Unknown error" make a comment about size_t needing to be bigger than 2 bytes define JINT_MAX and use it throw_ioe: don't treat negative values the same as positive ones fix a case where we got compiler warnings on 32-bit compiles because we tried to print a 64-bit number with '%d'
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12540175/HADOOP-8686.002.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1297//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1297//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12540175/HADOOP-8686.002.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1297//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1297//console This message is automatically generated.
        Hide
        Andy Isaacson added a comment -
        +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 bug.

        +  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.

        Show
        Andy Isaacson added a comment - +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 bug. + 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.
        Hide
        Colin Patrick McCabe added a comment -
        • SnappyCompressor: fix a case where we were passing in a pointer to a
          4-byte value rather than a pointer to a (usually) 8-byte value.
        • fix LZ4_compress prototype.
        • don't put junk on the line after an #endif – it triggers a compiler warning.
        • NativeIO: #define _GNU_SOURCE so that sync_file_range is not an
          implicitly declared function on Linux.
        • Java_org_apache_hadoop_io_compress_snappy_SnappyCompressor_initIDs:
          don't leak memory on error.
        • NativeIO: throw_ioe: we were using strerror_r, but assuming that it
          returned an int. However, it returns a char* in GNU glibc.
          (Yes, there was an #ifdef that was supposed to prevent this, but it was
          busted.) Instead, just use sys_errlist directly if we have an errno
          that is in range; otherwise come up with our own message. This should
          work on all platforms.
        • move a few declarations to the tops of functions. This was done so
          that error handling using goto didn't cause those variables to be used
          with undefined values. (This would happen if you used goto from a
          position before a C99-style variable declaration.)
        Show
        Colin Patrick McCabe added a comment - SnappyCompressor: fix a case where we were passing in a pointer to a 4-byte value rather than a pointer to a (usually) 8-byte value. fix LZ4_compress prototype. don't put junk on the line after an #endif – it triggers a compiler warning. NativeIO: #define _GNU_SOURCE so that sync_file_range is not an implicitly declared function on Linux. Java_org_apache_hadoop_io_compress_snappy_SnappyCompressor_initIDs: don't leak memory on error. NativeIO: throw_ioe: we were using strerror_r, but assuming that it returned an int. However, it returns a char* in GNU glibc. (Yes, there was an #ifdef that was supposed to prevent this, but it was busted.) Instead, just use sys_errlist directly if we have an errno that is in range; otherwise come up with our own message. This should work on all platforms. move a few declarations to the tops of functions. This was done so that error handling using goto didn't cause those variables to be used with undefined values. (This would happen if you used goto from a position before a C99-style variable declaration.)

          People

          • Assignee:
            Colin Patrick McCabe
            Reporter:
            Colin Patrick McCabe
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development