MINOR: Include the inner exception stack trace when re-throwing an exception

[divijvaidya]

Problem:

While wrapping the caught exception into a custom one, information about the caught exception is being lost, including information about the stack trace of the exception.

Change

When re-throwing an exception, we make sure to include the stack trace. Otherwise pertinent debug information is lost.

[divijvaidya]

@dengziming @hachikuji kindly review.

Reviewer(s) please note that all tests failing for this CI are known to be flaky and are not related to this code change.

Test 1 - testListenerConnectionRateLimitWhenActualRateAboveLimit - fix is pending PR review #12045

Test 2 - testTopicIdUpgradeAfterReassigningPartitions - fix is pending PR review #11687

Test 3 - testSnapshotOnlyAfterConfiguredMinBytes - (possible) fix is pending PR review #12224

Test 4 - Created a ticket at https://issues.apache.org/jira/browse/KAFKA-13951

[Kvicii]

LGTM.

[divijvaidya]

@hachikuji Please review. This should be a small one.

[dengziming]

Can this kind of problem be caught by spotbugs? manual checking is error prone.

[divijvaidya]

Can this kind of problem be caught by spotbugs? manual checking is error prone.

Agreed @dengziming but unfortunately spotbugs isn't catching such errors.

[divijvaidya]

@mimaison perhaps you may want to look into this? This already has 2 approvals from non-committers.

[ijuma]

Have we checked that we're not introducing a security vulnerability in these security classes by including the original exception?

[divijvaidya]

UnsupportedEncodingException is thrown for 2 cases and in either of the two cases, the string being encoded is not printed in the exception message and hence, we don't leak any message information using the exception.

Case 1: for UnsupportedCharsetException in which case, only charset is printed in the string as per the implementation as below:

public class UnsupportedCharsetException
    extends IllegalArgumentException
{

    private static final long serialVersionUID = 1490765524727386367L;

    private String charsetName;

    /**
     * Constructs an instance of this class.
     *
     * @param  charsetName
     *         The name of the unsupported charset
     */
    public UnsupportedCharsetException(String charsetName) {
        super(String.valueOf(charsetName));
	this.charsetName = charsetName;
    }

    /**
     * Retrieves the name of the unsupported charset.
     *
     * @return  The name of the unsupported charset
     */
    public String getCharsetName() {
        return charsetName;
    }

}

Case 2: for IllegalCharsetNameException which again, just prints the name of the charset. See implementation as below:

public class IllegalCharsetNameException
    extends IllegalArgumentException
{

    private static final long serialVersionUID = 1457525358470002989L;

    private String charsetName;

    /**
     * Constructs an instance of this class.
     *
     * @param  charsetName
     *         The illegal charset name
     */
    public IllegalCharsetNameException(String charsetName) {
        super(String.valueOf(charsetName));
	this.charsetName = charsetName;
    }

    /**
     * Retrieves the illegal charset name.
     *
     * @return  The illegal charset name
     */
    public String getCharsetName() {
        return charsetName;
    }

}

Is there any other security risk that you are alluding to here? Adding the stack trace is beneficial here to quickly determine whether the failure is due to case 1 or case 2.

[showuon]

Thanks for the explanation @divijvaidya , yes, I checked and confirmed it only output the charset name. So it's safe. @ijuma , any concern/comments here?

[divijvaidya]

@ijuma I have addressed your comments. This is ready for your review (pending test run).

[mdedetrich]

These changes seem good for me

[divijvaidya]

@tombentley requesting code review bandwidth from you here. We already have 3 non-committer approvals on this one.

[showuon]

LGTM! Thanks for the improvement.

[showuon]

Thanks for the explanation @divijvaidya , yes, I checked and confirmed it only output the charset name. So it's safe. @ijuma , any concern/comments here?

[divijvaidya]

Rebased with the latest trunk to resolve merge conflicts.

[showuon]

I'm going to merge this PR this week if there's no more comments. Thanks.

[ijuma]

LGTM, thanks.