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

EDF.decode overflows for large physical minimum/maximum #67

Open
kleinschmidt opened this issue Jun 26, 2023 · 1 comment
Open

EDF.decode overflows for large physical minimum/maximum #67

kleinschmidt opened this issue Jun 26, 2023 · 1 comment

Comments

@kleinschmidt
Copy link
Member

It multiplies by pmax - pmin which difference will overflow if you're within a factor of 2 of the maxfloat(Float32), and the multiplication will likely overflow if you're within ~32k (typemax(Int16)) of maxfloat(Float32).

However, I think that this is not really likely to be an issue; both our implementation and that used in PyMNE (EDFlib) write the physical min/max in a decimal form with at most 8 ASCII characters and no scientific notation, so the biggest value they can represent is well below the values that cause overflow here. So IMO it's better to fix this by adding a check to teh SignalHeader constructor that prevents you from constructing headers that would overflow; as a bonus it might be good to throw an error if the stringified physical min/max are >8 characters (rather than waiting until you try to actually write the file to find out).

If we don't wanna restrict the signal header to values that can be stringified into 8 ASCII characters, I think because we're first dividing by the digital range (max - min) in decode, we should probably err on the side of caution and prevent anything that would cause overflow for any integer dmin/dmax values (cna't rule out that someone would put some kind of cursed very small fractions in there but that gives you something on the order of 1e8, so 1e12 with the 1e4 from the typemax(Int16) before multiplying by the physical range. So if we cap those to 1e20 we'll be safe I think, and that still seems well outside the range of values you're likely to encounter in the real world...

@ararslan
Copy link
Member

xref #31

Tangentially relevant but EDF+ has a specification for how to handle values that are too large to be represented in 2 bytes. (Spoiler alert: you just take a log.)

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

No branches or pull requests

2 participants