Clojure

Three-arg ExceptionInfo constructor permits nil data

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Triaged

Description

The argument check in the two-arg clojure.lang.ExceptionInfo constructor isn't present in the three-arg constructor so it's possible to create an ExceptionInfo with arbitrary (or nil) data.

E.g.:

user=> (clojure-version)
"1.5.1"

user=> (ex-info "hi" nil)
IllegalArgumentException Additional data must be a persistent map: null  clojure.lang.ExceptionInfo.<init> (ExceptionInfo.java:26)

user=> (ex-info "hi" nil (Throwable.))
NullPointerException   clojure.lang.ExceptionInfo.toString (ExceptionInfo.java:40)

Patch: clj-1380-v3.patch

Screened by: Alex Miller

  1. clj-1380.diff
    27/Oct/14 5:30 AM
    3 kB
    Gordon Syme
  2. clj-1380-v2.patch
    05/Aug/15 5:51 PM
    3 kB
    Michael Blume
  3. clj-1380-v3.patch
    07/Aug/15 3:03 PM
    3 kB
    Gordon Syme

Activity

Hide
Gordon Syme added a comment - - edited

Done and done.
Throws on null rather than instanceof failure and updated the error message

Show
Gordon Syme added a comment - - edited Done and done. Throws on null rather than instanceof failure and updated the error message
Hide
Alex Miller added a comment -

Can we change the data instanceof IPersistentMap check to data != null (which is the real thing being checked? If something non-null and not IPersistentMap is passed, a ClassCastException will be thrown on invocation.

The error message should then be modified as well to something like "Additional data must be non-nil."

Show
Alex Miller added a comment - Can we change the data instanceof IPersistentMap check to data != null (which is the real thing being checked? If something non-null and not IPersistentMap is passed, a ClassCastException will be thrown on invocation. The error message should then be modified as well to something like "Additional data must be non-nil."
Hide
Michael Blume added a comment -

Rebased onto current master (1d5237)

Show
Michael Blume added a comment - Rebased onto current master (1d5237)
Hide
Gordon Syme added a comment - - edited

Whoops, sorry Andy.

I've rebased against master and added a correctly formatted patch.

Show
Gordon Syme added a comment - - edited Whoops, sorry Andy. I've rebased against master and added a correctly formatted patch.
Hide
Andy Fingerhut added a comment -

Gordon, I do not know if your patch is of interest to the Clojure developers, so I can't comment on that aspect of this ticket.

Instructions for creating a patch in the expected format is given on the wiki page below. Your patch is not in the expected format.

http://dev.clojure.org/display/community/Developing+Patches

Show
Andy Fingerhut added a comment - Gordon, I do not know if your patch is of interest to the Clojure developers, so I can't comment on that aspect of this ticket. Instructions for creating a patch in the expected format is given on the wiki page below. Your patch is not in the expected format. http://dev.clojure.org/display/community/Developing+Patches
Hide
Gordon Syme added a comment -

I just checked http://clojure.org/contributing, looks like my CCA made it through

Show
Gordon Syme added a comment - I just checked http://clojure.org/contributing, looks like my CCA made it through
Hide
Gordon Syme added a comment -

Hi Alex,

sure, that makes sense. I'll get the contributor's agreement in the post. It may take a while to arrive since I'm based in Europe.

Show
Gordon Syme added a comment - Hi Alex, sure, that makes sense. I'll get the contributor's agreement in the post. It may take a while to arrive since I'm based in Europe.
Hide
Alex Miller added a comment -

No worries on the classification - I adjust most incoming tickets in some way or another.

Thanks for the patch, however it cannot be considered unless you complete the Clojure Contributor's Agreement - http://clojure.org/contributing. This is an important step in the process that keeps the Clojure codebase on a sound legal basis.

Someone else could develop a clean room patch implementation for this ticket later, but of course it would be ideal if you could become a contributor!

Show
Alex Miller added a comment - No worries on the classification - I adjust most incoming tickets in some way or another. Thanks for the patch, however it cannot be considered unless you complete the Clojure Contributor's Agreement - http://clojure.org/contributing. This is an important step in the process that keeps the Clojure codebase on a sound legal basis. Someone else could develop a clean room patch implementation for this ticket later, but of course it would be ideal if you could become a contributor!
Hide
Gordon Syme added a comment - - edited

Patch + tests

I'm not at all familiar with the project so may have put tests in the wrong language and/or wrong place.

The ex-info-works test is a bit dorky but shows that both constructors are equivalent (and passes without the patch to ExceptionInfo).

Show
Gordon Syme added a comment - - edited Patch + tests I'm not at all familiar with the project so may have put tests in the wrong language and/or wrong place. The ex-info-works test is a bit dorky but shows that both constructors are equivalent (and passes without the patch to ExceptionInfo).
Hide
Gordon Syme added a comment -

Sorry, didn't meant to classify as "major" and I don't have permissions to edit.

Show
Gordon Syme added a comment - Sorry, didn't meant to classify as "major" and I don't have permissions to edit.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: