Hadoop Common
  1. Hadoop Common
  2. HADOOP-4467

SerializationFactory should use current context ClassLoader

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.18.1, 0.19.0
    • Fix Version/s: 0.18.2
    • Component/s: None
    • Labels:
      None
    • Release Note:
      SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances.

      Description

      SerializationFactory#add calls Class.forName without a ClassLoader instance. This prevents user-space Serialization classes from being loaded.

      The resulting ClassNotFoundException is eaten after being logged, and a NPE is thrown further down the line when a Serializer is requested.

      1. hadoop-4467-v2.patch
        2 kB
        Chris K Wensel
      2. hadoop-4467.patch
        1 kB
        Chris K Wensel

        Activity

        Chris K Wensel created issue -
        Chris K Wensel made changes -
        Field Original Value New Value
        Summary SerializationFactory should use current contextClassLoader SerializationFactory should use current context ClassLoader
        Affects Version/s 0.18.1 [ 12313357 ]
        Description SerializationFactory#add calls Class.forName without a ClassLoader instance. This prevents user-space Serialization classes from being loaded.

        The resulting ClassNotFoundException is eaten after being logged, and a NPE is thrown further down the line when a Serializer is requested.
        Hide
        Chris K Wensel added a comment -

        I probably should have phrased this one differently, but it does seem to be standard practice to provide the context classloader when calling Class.forName.

        I can submit a patch with the one line change. but I do not know if there should (or even could be) an accompanying unit test for this. please advise.

        Show
        Chris K Wensel added a comment - I probably should have phrased this one differently, but it does seem to be standard practice to provide the context classloader when calling Class.forName. I can submit a patch with the one line change. but I do not know if there should (or even could be) an accompanying unit test for this. please advise.
        Hide
        Chris K Wensel added a comment -

        added 0.19 as an affected version

        Show
        Chris K Wensel added a comment - added 0.19 as an affected version
        Chris K Wensel made changes -
        Affects Version/s 0.19.0 [ 12313211 ]
        Hide
        Chris K Wensel added a comment -

        initial attempt, without unit tests

        Show
        Chris K Wensel added a comment - initial attempt, without unit tests
        Chris K Wensel made changes -
        Attachment hadoop-4467.patch [ 12392530 ]
        Chris K Wensel made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Release Note SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances.
        Hide
        Owen O'Malley added a comment -

        Wouldn't it be better to use the class loader associated with the configuration rather than the current thread? Basically, use

           conf.getClassByName(name);
        

        Clearly the conf for the serializer would need to be saved from the constructor.

        Show
        Owen O'Malley added a comment - Wouldn't it be better to use the class loader associated with the configuration rather than the current thread? Basically, use conf.getClassByName(name); Clearly the conf for the serializer would need to be saved from the constructor.
        Chris K Wensel made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Chris K Wensel added a comment -

        agreed.

        Show
        Chris K Wensel added a comment - agreed.
        Hide
        Chris K Wensel added a comment -

        delegates classloading back to Configuration

        Show
        Chris K Wensel added a comment - delegates classloading back to Configuration
        Chris K Wensel made changes -
        Attachment hadoop-4467-v2.patch [ 12392536 ]
        Hide
        Chris K Wensel added a comment -

        delegates back to Configuration

        Show
        Chris K Wensel added a comment - delegates back to Configuration
        Chris K Wensel made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Tom White added a comment -

        +1

        Show
        Tom White added a comment - +1
        Hide
        Chris K Wensel added a comment -

        All but org.apache.hadoop.fs.TestLocalDirAllocator tests pass.

        I think this is unrelated:
        shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory

        Show
        Chris K Wensel added a comment - All but org.apache.hadoop.fs.TestLocalDirAllocator tests pass. I think this is unrelated: shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
        Hide
        Chris K Wensel added a comment -

        via ant test-patch...

        seems to be down on lack of tests. unsure, per previous comment, how to test classloader issues.

        [exec] -1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
        [exec] Please justify why no tests are needed for this patch.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]

        Show
        Chris K Wensel added a comment - via ant test-patch... seems to be down on lack of tests. unsure, per previous comment, how to test classloader issues. [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec]
        Chris K Wensel made changes -
        Priority Major [ 3 ] Blocker [ 1 ]
        Hide
        Arun C Murthy added a comment -

        I just committed this. Thanks, Chris!

        Show
        Arun C Murthy added a comment - I just committed this. Thanks, Chris!
        Arun C Murthy made changes -
        Fix Version/s 0.18.2 [ 12313424 ]
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Assignee Chris K Wensel [ cwensel ]
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk #640 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/640/)
        . SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances. Contributed by Chris Wensel.

        Show
        Hudson added a comment - Integrated in Hadoop-trunk #640 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/640/ ) . SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances. Contributed by Chris Wensel.
        Nigel Daley made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Patch Available Patch Available Open Open
        26m 47s 1 Chris K Wensel 21/Oct/08 00:47
        Open Open Patch Available Patch Available
        1h 1m 2 Chris K Wensel 21/Oct/08 01:00
        Patch Available Patch Available Resolved Resolved
        2d 3h 52m 1 Arun C Murthy 23/Oct/08 04:53
        Resolved Resolved Closed Closed
        12d 14h 55m 1 Nigel Daley 04/Nov/08 18:48

          People

          • Assignee:
            Chris K Wensel
            Reporter:
            Chris K Wensel
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development