Array out of bounds in Controller.h

[GoogleCodeExporter]

http://pastebin.com/kDhX9a5b

In the function GetController, an invalid index, despite being checked and 
logged, still returns an invalid reference. 

Possible solutions:
1. a NULLController (Controller0?) could be returned, which prints error 
messages if used and has no influence on the screen
2. Instead of returning references, you could return pointers and then return a 
nullptr
3. boost::optional could be used to indicate the controller will not 
necessarily be returned.
4. An assertion could crash it when used in debug

Original issue reported on code.google.com by LoveOver...@gmail.com on 21 Sep 2013 at 2:16

[GoogleCodeExporter]

Good catch. 

Original comment by lieseg...@gmail.com on 26 Sep 2013 at 3:38

• Changed state: Accepted

[GoogleCodeExporter]

Fixed in repo. 

Original comment by lieseg...@gmail.com on 27 Sep 2013 at 3:29

• Changed state: Fixed

[GoogleCodeExporter]

Your fix in the repo is not acceptable; it calls assert in debug mode, but 
still triggers an array out of bounds in release mode.

Any number except 0 and 1 will trigger one of three conditions:
1. an immediate access violation, triggering "terminate" and causing the 
program to hard abort
2. random memory to be pulled into a reference without a failure, causing 
ticking time bomb to develop, making debugging essentially impossible
3. random memory to be pulled into a reference without a failure, but the 
memory is sufficiently close that no failures happen, but strange and 
inscrutable side effects to occur

None of these is acceptable. If you must return a reference, you should default 
to some NullController, or return the first player controller. A pointer would 
allow you to return NULL and prevent this issue, as would throwing an exception.

Furthermore, the assert(0) returns absolutely no information about what caused 
the issue, making debugging harder. assert(0 && "Controller index out of 
bounds") would be more acceptable. 

Original comment by LoveOver...@gmail.com on 27 Sep 2013 at 3:50

[GoogleCodeExporter]

The assert was listed as an acceptable solution in your bug; combined with the 
error message, it should be enough for a programmer to determine what happened. 

Original comment by lieseg...@gmail.com on 27 Sep 2013 at 3:58

[GoogleCodeExporter]

To be more specific: the more thorough changes you suggest probably *would* be 
better, but would require re-architecting things in a way I'm not comfortable 
doing at this point in Angel's evolution. It's the kind of thing that could be 
on the list for Angel 4, however, which I'm currently seeing as a big 
refactor/cleanup as it will also be introducing multiple windows, networking, 
etc. 

Original comment by lieseg...@gmail.com on 27 Sep 2013 at 4:11

[GoogleCodeExporter]

The assert was listed as the lowest on a priority of four items, and does not 
fully fix the situation in release; while it provides much more helpful than a 
random undiagnosable crash, it is still not an acceptable solution on its own.

Restricting the domain of the function and/or returning an acceptable default 
is a full solution, and is not that difficult of a change. If you do not wish 
to change the interface, simply forcing controllerIndex to 0 would be 
acceptable.

Original comment by LoveOver...@gmail.com on 27 Sep 2013 at 4:15

[GoogleCodeExporter]

Forcing the controllerIndex to 0 would still provide junk data if there were no 
controllers plugged in. 

The controller interface isn't optimal, for sure, but this is one of those 
areas where we rely on client code to know what it's doing -- basically, the 
callers of this function should be doing a check to ensure that they aren't 
passing it an invalid index. 

Original comment by lieseg...@gmail.com on 27 Sep 2013 at 1:19

[GoogleCodeExporter]

I understand the need to not break architecture, but the idea that it's not the 
libraries duty to satisfy/check for reasonable pre and post conditions is one 
I'd refer to jokingly as "morally reprehensible" (a phrase I'll surely use when 
I show how messaging is broken). 

The client code shouldn't need to dig through internals of objects to figure 
out why it gets the wrong result or a crash from passing a perfectly reasonable 
request in, even under different conditions (such as controllers not passed in, 
etc). Especially not if a design goal is "easy to understand code".

Original comment by LoveOver...@gmail.com on 27 Sep 2013 at 2:00

[GoogleCodeExporter]

There's no need to dig through code -- it prints an error message telling you 
what went wrong. It lets you make the decision about whether you want to keep 
moving and focus on something else, or fix it right away. 

Remember with all these things that Angel is a prototyping system first and 
foremost. Certain errors are tolerable so long as the programmer is told about 
them. 

Original comment by lieseg...@gmail.com on 27 Sep 2013 at 2:25