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.

Sign up for GitHub

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

Replace deprecated Network usage #25

Merged
merged 5 commits into from
Jun 19, 2019
Merged

Replace deprecated Network usage #25

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd29910
Add network-bsd
shulhi May 24, 2019
31a4fd7
Replace deprecated Network usage
shulhi May 24, 2019
ab400d8
Fix crlf and indentations
shulhi May 24, 2019
cb3da1f
Read buffer till empty instead of reading lines with max bytes
shulhi May 24, 2019
91eed69
Use connectionGetLine and deprecate redundant functions
shulhi Jun 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Network/Mail/SMTP.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,14 @@ parseResponse conn = do
(code, bdy) <- readLines
return (read $ B8.unpack code, B8.unlines bdy)
where
getLines c acc = do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parsonsmatt I need some help with this as I am not sure if this is right or should I just stick with connectionGetLine. Having to specify max bytes per line seems a little off to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thinking out loud here:

  • This function is replacing B8.hGetLine :: Handle -> IO ByteString. Implementation linked.
  • This function reads from the bytestring until it either finds a \n character or the end-of-file. (figuring this out exactly took me a long time, dang the bytestring internals are tough)
  • connectionGetLine doesn't work in exactly the same way. The Int parameter is to prevent DoS attacks, so it'd be a security improvement to have it for sure - no idea what typical line length limit might be safe here 🤷‍♂️ .
  • The bigger problem with connectionGetLine is that it throws isEOFError exception on end of input. Except, it says "An end of file will be considered as a line terminator too, if line is not empty." - Looking at the source... well, it's a bit confusing, but I think I've worked out what is going on.

I think we're fine with connectionGetLine, we just need to pick a suitable line length.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did a quick Google and got this StackOverflow post re line lengths in mail.

I think 1000 is a reasonable pick. The standard says lines MUST be no more than 998 characters.

res <- Conn.connectionGetChunk c
if B8.null res
then return acc
else getLines c $ B8.append acc res
Copy link
Collaborator

Choose a reason for hiding this comment

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

This current implementation does not return a line, but instead returns the entire contents of the connection. Replacing it with connectionGetLine will fix it.


readLines = do
l <- Conn.connectionGetLine (16 * 1024) conn
l <- getLines conn B8.empty
let (c, bdy) = B8.span isDigit l
if not (B8.null bdy) && B8.head bdy == '-'
then do (c2, ls) <- readLines
Expand Down