View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0000641 | LDMud 3.5 | Implementation | public | 2009-05-26 03:03 | 2018-01-30 03:59 |
| Reporter | zesstra | Assigned To | zesstra | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Platform | x86_64 | OS | MacOS X | OS Version | 10.5.x |
| Target Version | 3.5.0 | Fixed in Version | 3.5.0 | ||
| Summary | 0000641: save_object() should never write NaN or INF into savefiles. | ||||
| Description | If a float is actually too large to be represented and it is saved in save_object()/save_value(), a restore with restore_object()/restore_value() will fail (not only for the float, but for the complete savefile). A similar issue is "NaN". restore_svalue() expects numbers to start with [0-9,-], but sprintf("%.12e") with my libc will print infinity as 'inf' (and NaN probably as 'nan'/'NaN'). C99 specifies that sprintf uses "[-]inf" or "[-]infinity" for infinity and a string starting with "nan" for NaN when printing floating point numbers. One idea is, to define a new savefile format: a) with a special first character as a tag for floats. The downside is, that the savefiles gets a little bit bigger and as less readable for humans. b) never write the native float representation ("%.12e") into the savefile, only use separate mantissa+exponent. The downside is, the savefiles get even less readable for humans as well. Additionally I am not completely sure, what implications this has for portability. Right now we have only on floating point implementation, but maybe that changes in the future and creating yet another savefile format then is not something I look forward to... | ||||
| Tags | No tags attached. | ||||
|
|
Thanks for separating this issue out from 0000627. I was wondering how NaNs and infs would end up in the save file in the first place, ignoring 0000627 -- the driver seems to be careful to not create such values and turns them into errors instead. |
|
|
Mhmm. Very good point. And preventing overflows/underflows in LPC is a good idea. I guess you are right, we might just define any case of a float being inf or NaN a bug somewhere else. The only downside of that is, that savefiles may be written, which can't be restored. We could check that in save_svalue() and raise an error in that case to prevent writing illegal savefiles... |
|
|
I think, I concur with Fuchur, that 'infinity' and 'NaN' should not be in savefiles in the first place, if they are, there is a problem somewhere else. So I suggest to close this. Any more opinions? |
|
|
I think its a good idea to not serialize the values for NaN and Infinity as they are somewhat special. so is NaN always != NaN ... I wonder how this is handled after restoring the objects value. From my opinion, making it an error is correct, as it obviously is one. I cannot think of somebody really taking advantage of saving NaN or Inf. |
|
|
> I wonder how this is handled after restoring the objects value. Well, it is not handled, because we can't restore savefiles with such values. ;-) You just get an error about the savefile having an illegal format (or such). As Fuchur writes, the driver usually does not create those values in the first place (0000627 was special because due to architecture issues a float was mis-interpreted during save_svalue()). But currently, we don't have an active check in save_svalue() for these. |
|
|
Do we need such a check (for NaN, infinity) in save_svalue() at the time we write the savefile? |
|
|
I say: Yes, we need such a check, and the try to save Inf or NaN values should result in an error. In a mud, I can't imagine a use case where it is needed to write Inf or NaN into a savefile. I even think that a mudlib's need to store such a value in a savefile does more seem to be a design error. |
|
|
I agree concerning the need of these values Coogan, but the argument is: there should be no situation the driver writes NaN or infinity into an svalue. Therefore no check in save_svalue() is necessary. If the driver creates such an svalue, that itself is an error that has to be fixed. The only argument is: we want the check because we are paranoid. ;-) |
|
|
I will add an assert in save_svalue() checking that the double written are finite. That will prevent invalid savefiles even if the driver has an error somewhere else. That should make all of us paranoid people happy. ;-) |
|
|
Fix committed in revision e8ad384d5c9f26f2675ff7bd2c1fee66772a201f to master branch (see changeset 937 for details). Thank you for reporting! |
|
|
Fix committed in revision e8ad384d5c9f26f2675ff7bd2c1fee66772a201f to master branch (see changeset 1437 for details). Thank you for reporting! |
|
|
Fix committed in revision e8ad384d5c9f26f2675ff7bd2c1fee66772a201f to master branch (see changeset 2765 for details). Thank you for reporting! |
|
|
Fix committed in revision e8ad384d5c9f26f2675ff7bd2c1fee66772a201f to master branch (see changeset 3850 for details). Thank you for reporting! |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2009-05-26 03:03 | zesstra | New Issue | |
| 2009-05-26 03:08 | zesstra | Relationship added | related to 0000627 |
| 2009-05-26 10:03 | fufu | Note Added: 0001163 | |
| 2009-05-27 03:32 | zesstra | Note Added: 0001167 | |
| 2009-10-28 05:15 | zesstra | Note Added: 0001567 | |
| 2009-10-28 05:15 | zesstra | Status | new => feedback |
| 2009-10-30 03:00 | Bardioc | Note Added: 0001573 | |
| 2009-10-30 04:11 | zesstra | Note Added: 0001574 | |
| 2010-02-25 15:23 | zesstra | Summary | restore_object() should be able to restore floats having the value 'infinity' and 'NaN'. => save_object() should never write NaN or INF into savefiles. |
| 2010-03-14 16:43 | zesstra | Note Added: 0001799 | |
| 2011-02-19 17:54 | zesstra | Target Version | 3.3.720 => 3.3.721 |
| 2011-02-24 00:46 | Coogan | Note Added: 0002036 | |
| 2012-12-09 01:59 | zesstra | Note Added: 0002175 | |
| 2012-12-09 01:59 | zesstra | Status | feedback => new |
| 2012-12-09 01:59 | zesstra | Note Edited: 0002175 | |
| 2014-02-23 22:05 | zesstra | Note Added: 0002230 | |
| 2014-02-23 22:05 | zesstra | Assigned To | => zesstra |
| 2014-02-23 22:05 | zesstra | Status | new => assigned |
| 2014-02-23 22:15 |
|
Source_changeset_attached | => ldmud.git master e8ad384d |
| 2014-02-23 22:15 |
|
Note Added: 0002231 | |
| 2014-02-23 22:15 |
|
Assigned To | zesstra => ldmud-bugs |
| 2014-02-23 22:15 |
|
Status | assigned => resolved |
| 2014-02-23 22:15 |
|
Resolution | open => fixed |
| 2014-02-23 22:19 | zesstra | Project | LDMud 3.3 => LDMud 3.5 |
| 2014-02-23 22:20 | zesstra | Assigned To | ldmud-bugs => |
| 2014-02-23 22:20 | zesstra | Product Version | 3.3.718 => |
| 2014-02-23 22:20 | zesstra | Fixed in Version | => 3.5.0 |
| 2014-02-23 22:20 | zesstra | Target Version | 3.3.721 => 3.5.0 |
| 2014-02-23 22:20 | zesstra | Assigned To | => zesstra |
| 2018-01-29 18:59 | zesstra | Source_changeset_attached | => ldmud.git master e8ad384d |
| 2018-01-29 18:59 | zesstra | Note Added: 0002314 | |
| 2018-01-29 21:57 | zesstra | Source_changeset_attached | => ldmud.git master e8ad384d |
| 2018-01-29 21:57 | zesstra | Note Added: 0002365 | |
| 2018-01-30 03:59 | zesstra | Source_changeset_attached | => ldmud.git master e8ad384d |
| 2018-01-30 03:59 | zesstra | Note Added: 0002416 |
