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

strtod might be unsafe on some systems #61

Open
boazsegev opened this issue May 27, 2019 · 6 comments
Open

strtod might be unsafe on some systems #61

boazsegev opened this issue May 27, 2019 · 6 comments
Assignees

Comments

@boazsegev
Copy link
Owner

boazsegev commented May 27, 2019

facil.io protects the JSON parser and Strings from overflowing during a call to strtod... however, it still appears that strtod calls strlen internally on some systems.

One might have assumed the issue was fixed... however, using my profiler showed distinct calls make from strtod to strlen on some systems (and I hope it was some sort of mistake).

This isn't really safe if the string isn't NUL terminated.

It also wastes CPU cycles when the floating point number is in the middle (rather than the end) of a string.

Rolling an strtod alternative (or copying the buffer to a NUL terminated buffer before parsing the data), should be considered.

It is currently recommended that developers test the strtod implementations and consider overriding the function using safe implementations (when discovering an issue).

@frink
Copy link

frink commented May 27, 2019

Could we copy an existing implementation from one of the many libs around?
Or would it be better to simplify for our single use case?

@boazsegev
Copy link
Owner Author

@frink ,

You're probably right. Finding a good implementation with an MIT license and integrating it is would probably be the most effective approach - especially if we could use the makefile script to install it directly from the original repo.

However, there's a couple of reasons I'm holding off on that:

  1. I want to support binary representation, (0b01...), parsing doubles using their binary IEEE 754 representation. The fio_atol adds support for binary numbers and I thought fio_atof should follow suite.

  2. I love to understand the code that I use, just so I can better maintain the project. That's why I prefer (when possible) to give it a try myself and solve what I can before integrating someone else's code / approach.

Having said that, adding a task to the makefile, like the one for BearSSL, would be a great solution.

@boazsegev boazsegev changed the title strtod should be considered unsafe strtod might be unsafe on some systems May 27, 2019
@frink
Copy link

frink commented May 28, 2019

Musl is MIT. Don't know if it supports binary representations or not...

This one is public domain but may need a little finesse:
https://gist.github.com/mattn/1890186

@boazsegev
Copy link
Owner Author

boazsegev commented May 28, 2019

Thanks for taking the time to find a solution.

Lets put this one on low priority, there's a bunch of stuff that uses strtod and not all systems have issues in their implementations.

I think BearSSL would be more important than this side-detail.

Also...

Musl (edit: the public domain version in the link) suffers from more rounding errors my current version suffers from (my current, un-uploaded version, has 52bit precision on the mantissa instead of 53 bit precision, which is my only issue).

In addition, Musl (edit: the public domain version in the link) doesn't support hex notation, NaN and Infinity... which I think could be important (my version also supports binary notation, but I didn't finish the hex notation yet).

@boazsegev
Copy link
Owner Author

Correction: I was looking at the public domain implementation. The Musl library might be a good option for an strtod patch. I'll review it later.

@frink
Copy link

frink commented May 28, 2019

Good dea!

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

No branches or pull requests

2 participants