Bug 3609 – isNumeric causes a stack overlfow with mutable arrays

Test case: import std.string; void main(string[] args) { isNumeric("1".dup); } This results in a stack overflow on DMD 2.037. Note that immutable arrays work correctly. (i.e. isNumeric("1".idup); is okay)

I ran into this a few days, too and was meaning to file it. isNumeric is **completely** busted on D2, and it amazes me that noone noticed it until now. The problem is that isNumeric(string, bool) is overloaded against isNumeric(...). In D1, string is an alias for char[]. In D2, it's an alias for immutable(char)[]. If the type that's input is not a string, it's sent to isNumeric(...). If it's a mutable char[] array in D2, the following is executed: if (_arguments[0] == typeid(char[])) return isNumeric(va_arg!(char[])(_argptr)); Obviously, the intent here is to call the overload, but in D2 the overload expects an immutable(char)[], leading to isNumeric(...) getting called again. Violia, infinite recursion and stack overflows. I doubt anyone actually uses isNumeric(...), because its functionality should really be handled at compile time w/ templates. I doubt there are too many use cases for having it be handled at runtime. Therefore, I recommend dumping it and templating isNumeric(string) to handle char[], wchar[], dchar[], and the const and immutable flavors of these. This would also fix bug 3610.

(In reply to comment #1) > I ran into this a few days, too and was meaning to file it. isNumeric is > **completely** busted on D2, and it amazes me that noone noticed it until now. > The problem is that isNumeric(string, bool) is overloaded against > isNumeric(...). In D1, string is an alias for char[]. In D2, it's an alias > for immutable(char)[]. If the type that's input is not a string, it's sent to > isNumeric(...). If it's a mutable char[] array in D2, the following is > executed: > > if (_arguments[0] == typeid(char[])) > return isNumeric(va_arg!(char[])(_argptr)); > > Obviously, the intent here is to call the overload, but in D2 the overload > expects an immutable(char)[], leading to isNumeric(...) getting called again. > Violia, infinite recursion and stack overflows. > > I doubt anyone actually uses isNumeric(...), because its functionality should > really be handled at compile time w/ templates. I doubt there are too many use > cases for having it be handled at runtime. Therefore, I recommend dumping it > and templating isNumeric(string) to handle char[], wchar[], dchar[], and the > const and immutable flavors of these. This would also fix bug 3610. IMHO it's kind of silly to define isNumeric. It does about the same amount of work as to, but doesn't give you the actual number. I suggest we just yank it.

(In reply to comment #2) > IMHO it's kind of silly to define isNumeric. It does about the same amount of > work as to, but doesn't give you the actual number. I suggest we just yank it. The only problem I see with this is that then you're using exception handling as flow control if your goal was just to validate something. Then again, this might not be a big deal because: 1. Why would you be validating something as being numeric if you have no intention of converting it to a number? 2. One reason why using exceptions as flow control is considered bad is performance. Not too many people try to convert strings to numbers inside performance critical code. 3. The other reason why exceptions as flow control is considered bad is that it's unstructured flow control, kind of like goto. However, in this case I don't think that applies. real foo; try { foo = to!real(someString); } catch(ConvError) { // yada yada yada } Seems pretty clear and readable to me. IMHO taking it out is probably a good move but I think we should think twice and allow others to comment b/c it does have the downside of encouraging using exceptions as flow control.

I can see from the year 2009 checkout that isNumeric took a string, but now it takes a const(char)[] s, and this seems to fix the issue. I'm not having stack overflows here. I'll mark this as fixed.