Skip to content
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

Fix infinite loops in util.decode #3466

Merged
merged 4 commits into from Jun 24, 2020
Merged

Conversation

rossabaker
Copy link
Member

@rossabaker rossabaker commented Jun 2, 2020

  • Replaces decode with an internal version with a RaiseThrowable constraint.
  • Check the error codes on each operation. Raise errors on malformed input, rather than loop infinitely.
  • The new version allocates one CharBuffer and ByteBuffer, and should be more efficient.
  • Media#bodyAsText is renamed to Media#bodyText, so we can add the constraint.
  • EntityDecoder.decodeString renamed to Entity.decodeText for the same reason.

Fixes #3453.

body.through(util.decode(cs))
}

final def bodyText(implicit RT: RaiseThrowable[F], defaultCharset: Charset = DefaultCharset): Stream[F, String] =
Copy link
Member Author

@rossabaker rossabaker Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this implicit charset pattern that we have. Maybe it's time to rip that out.

def decodeString[F[_]](
m: Media[F])(implicit F: Sync[F], defaultCharset: Charset = DefaultCharset): F[String] =
m.bodyAsText.compile.string

/** Decodes a message to a String */
def decodeText[F[_]](
m: Media[F])(implicit F: Sync[F], defaultCharset: Charset = DefaultCharset): F[String] =
Copy link
Member Author

@rossabaker rossabaker Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a case where we could take a RaiseThrowable[F] and Compiler[F, F] as evidence, since Sync is offensive. A Compiler[F, G] would be neat, but the G would ripple everywhere.

Pull.raiseError(new UnmappableCharacterException(result.length()))
}
case Some((chunk, stream)) =>
val chunkWithoutBom = skipByteOrderMark(chunk)
Copy link
Contributor

@hamnis hamnis Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this run on every chunk? or am I reading this wrong?

Copy link
Member Author

@rossabaker rossabaker Jun 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, and that's a pre-existing defect, which we were reminded of on the original ticket. I haven't had time to clean that up, but I wouldn't mind merging this while someone finds time to fix that.

@rossabaker rossabaker added this to the 0.21.5 milestone Jun 23, 2020
hamnis
hamnis approved these changes Jun 24, 2020
@rossabaker rossabaker merged commit 1234353 into http4s:series/0.21 Jun 24, 2020
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants