Re: Unsolicited GNU bc patch
Philip, thank you,
I'm sorry: I have sent this to upstream, but haven't heard anything from them.
At least with a mailing list, I get feedback as to whether or not my mail was
eaten by the void of the Internet. Also, if it gets into Debian, then the
patches filter through to everything based on Debian.
Philip, your change to the patch looks right. Sorry, I based the patch off
upstream. You do say it needs a better description, so I'm going to try to
give you a sense of what's going on.
What I think is happening is that, somewhere in the parser, "that an error
occurred" is getting suppressed, and the parser continues to generate bytecode
with the previous instruction incomplete, and then it tries to execute that.
Sometimes, the bytecode reads an instruction while trying to read a reference.
This appears to be most catastrophic in array handling. While what ought to be
fixed is the code generation to not generate these erroneous references, it is
easier to fix the bytecode interpreter to defend itself from them.
To begin, starting in execute.c: for change one, it has read a label number,
but then walks off the list looking for it. In change two, sometimes the
function number is invalid. And change three protects from the string not
being terminated. Looking at this again, if I had just added an 'else' to
"if (ch != '\\')" then I could have made a less invasive change. Also: if you
want to give any of these error messages better text, or if I've broken the
internationalization with them, please change them to suite your preferences.
What I gave you is better than the "DANGER, WILL ROBINSON!" that I had before.
In storage.c, initializing 'v_next' is one of the things I consider a bug.
Sometimes, it has a "valid" pointer in it. The next six changes are defensive
error checks to ensure that the array being requested is plausible. The line
"params++;" looks like a hold-over from an earlier version of the code, where
the parameters were stored in an array. With the linked-list, the proper way
to advance to the next parameter is "params = params->next;", which always
occurs a few lines later.
That leaves util.c. I think they were trying to save memory, at some point.
Possibly: variable names are treated differently from array and function
names, and I don't see the reason for that. What happens is that the value
from lookup() is used to initialize av_name in nextarg(). Then, av_name is
directly used to index v_names right above that removed "params++;" line. In
this retrospective dive through the code, that may be it. The line in
storage.c could be changed, I think, but, in my opinion, it is better to move
the code so that it more consistently handles all types. In addition, while
the line "if (id->v_name <= MAX_STORE)" is annoying in that it is different,
it isn't guarding against an invalid access.
Thank you again,
Thomas DiModica
Reply to: