New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CGI.unescapeHTML => Java::JavaLang::ArrayIndexOutOfBoundsException #4605

Closed
mmmries opened this Issue May 11, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@mmmries

mmmries commented May 11, 2017

Related to #4556

Environment

  • jruby 9.1.7.0 (2.3.1) 2017-01-11 68056ae Java HotSpot(TM) 64-Bit Server VM 25.60-b23 on 1.8.0_60-b27 +jit [darwin-x86_64]
  • MacOS Sierra

Expected Behavior

jruby-9.1.7.0 :008 > ::CGI.unescapeHTML(::JSON.parse("{\"str\":\"&#8220GOOD LOVIN&#82\"}")["str"])
=> "&#8220GOOD LOVIN&#82"

Actual Behavior

jruby-9.1.7.0 :008 > ::CGI.unescapeHTML(::JSON.parse("{\"str\":\"&#8220GOOD LOVIN&#82\"}")["str"])
Java::JavaLang::ArrayIndexOutOfBoundsException: 20
	from org.jruby.ext.cgi.escape.CGIEscape.optimized_unescape_html(CGIEscape.java:181)
	from org.jruby.ext.cgi.escape.CGIEscape.cgiesc_unescape_html(CGIEscape.java:372)
	from org.jruby.ext.cgi.escape.CGIEscape$INVOKER$s$1$0$cgiesc_unescape_html.call(CGIEscape$INVOKER$s$1$0$cgiesc_unescape_html.gen)
	from org.jruby.runtime.callsite.CachingCallSite.cacheAndCall(CachingCallSite.java:338)
	from org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:163)
	from org.jruby.ir.interpreter.InterpreterEngine.processCall(InterpreterEngine.java:315)
	from org.jruby.ir.interpreter.StartupInterpreterEngine.interpret(StartupInterpreterEngine.java:73)
	from org.jruby.ir.interpreter.Interpreter.INTERPRET_EVAL(Interpreter.java:122)
	from org.jruby.ir.interpreter.Interpreter.evalCommon(Interpreter.java:176)
	from org.jruby.ir.interpreter.Interpreter.evalWithBinding(Interpreter.java:200)
	from org.jruby.RubyKernel.evalCommon(RubyKernel.java:1033)
	from org.jruby.RubyKernel.eval19(RubyKernel.java:1000)
	from org.jruby.RubyKernel$INVOKER$s$0$3$eval19.call(RubyKernel$INVOKER$s$0$3$eval19.gen)
	from org.jruby.runtime.callsite.CachingCallSite.callBlock(CachingCallSite.java:77)
	from org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:83)
	from org.jruby.ir.instructions.CallBase.interpret(CallBase.java:428)
... 154 levels...
	from org.jruby.internal.runtime.methods.JavaMethod$JavaMethodOneOrNBlock.call(JavaMethod.java:383)
	from org.jruby.internal.runtime.methods.AliasMethod.call(AliasMethod.java:61)
	from org.jruby.runtime.callsite.CachingCallSite.cacheAndCall(CachingCallSite.java:338)
	from org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:163)
	from bin.rails.invokeOther8:require(bin/rails:8)
	from bin.rails.RUBY$script(bin/rails:8)
	from java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:627)
	from org.jruby.ir.Compiler$1.load(Compiler.java:90)
	from org.jruby.Ruby.runScript(Ruby.java:823)
	from org.jruby.Ruby.runNormally(Ruby.java:742)
	from org.jruby.Ruby.runNormally(Ruby.java:760)
	from org.jruby.Ruby.runFromMain(Ruby.java:573)
	from org.jruby.Main.doRunFromMain(Main.java:417)
	from org.jruby.Main.internalRun(Main.java:305)
	from org.jruby.Main.run(Main.java:232)
	from org.jruby.Main.main(Main.java:204)

Similar to the last issue I reported around this, I can't trigger the invalid behavior with a simple copy-paste of the string into IRB, but if I parse it out of a JSON structure (or a protobuf message in my application) then it fails to unescape cleanly.

@enebo enebo added this to the JRuby 9.1.9.0 milestone May 11, 2017

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 11, 2017

Member

Ok seems like when I audited after the last fix I missed the continue clause which change 'i' (thus requiring one more i < len check). Fix coming up.

Member

enebo commented May 11, 2017

Ok seems like when I audited after the last fix I missed the continue clause which change 'i' (thus requiring one more i < len check). Fix coming up.

@enebo enebo closed this in c7dfb2d May 11, 2017

@mmmries

This comment has been minimized.

Show comment
Hide comment
@mmmries

mmmries May 11, 2017

👏 That was some 🏎 speed coding @enebo. I'll pull it down and run in it through several million strings in our production system to see if I can find any other edge cases.

mmmries commented May 11, 2017

👏 That was some 🏎 speed coding @enebo. I'll pull it down and run in it through several million strings in our production system to see if I can find any other edge cases.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo May 11, 2017

Member

@mmmries great current plan for 9.1.9 is Monday so please beat this up :)

Member

enebo commented May 11, 2017

@mmmries great current plan for 9.1.9 is Monday so please beat this up :)

@mmmries

This comment has been minimized.

Show comment
Hide comment
@mmmries

mmmries May 11, 2017

I've pushed 942k strings through this so far (include all of the edge cases I had noticed previously) and everything looks good. I'll let you know if I find anything else, but I think it's probably good to release with 9.1.9.0

mmmries commented May 11, 2017

I've pushed 942k strings through this so far (include all of the edge cases I had noticed previously) and everything looks good. I'll let you know if I find anything else, but I think it's probably good to release with 9.1.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment