[fpc-devel] Const optimization is a serious bug

[Martin]

On 05/07/2011 04:02, Chad Berchek wrote:
>
> Martin wrote:
>> I don't think it is a bug.
> ...
>> "(const s: string)" is a declaration by the programmer, that the
>> variable (the string) will not be modified at all. That is neither by
>> the procedure called, nor by any code outside the procedure, via any
>> other  reference to the data. If the programmer sticks to what he
>> declared, then it works, if not it crashes.
>
> Is that true? I am not necessarily asserting that your statement is 
> false; it could well be true. However I personally have not seen it 
> documented so if anyone has a reference I would like to see it. As far 
> as I have been able to tell from the docs of Delphi and FPC, the const 
> declaration does NOT mean the programmer promises to not modify the 
> instance; it means he promises to not modify the variable, which the 
> compiler does indeed enforce.

Well, I have pointed out myself,in my mail, that it probably needs more 
documentation. I do not know if it is documented or not.

But it is the answer, I have gotten several times from developers in the 
FPC team. So for all I know it is the intended behaviour. At least 
intended in FPC (and apparently either intended or at least implemented 
in Delphi). So if there is no documentation for it, then it would appear 
a problem of documentation, rather than a bug in the compiler (Again all 
based on the statements I was given)


>> Anyway, if you do not like the feature do not use it.
>
> Unfortunately, that is not true. I don't have to use const in my own 
> code, but I cannot control the fact that it is already widely used in 
> other libraries, including those of FPC.

Well yes. But that is not a problem limited to the "const string param". 
Any code you use in your app, can contain any kind of bug.

> I just want to ask, after looking at the demo, is this not the least 
> bit scary to anyone? I mean, you can make one very simple, tiny change 
> and turn a perfectly working program into an immediate crash for no 
> apparent reason. The secrets are buried in there. In a real world 
> scenario you wouldn't be able to see it crash so easily. It may or may 
> not crash, or it may just crash much later, or it may not crash but 
> just be a security vulnerability waiting for someone to discover.
Very true, in fact the whole issue has only recently been discovered in 
the LCL too (as a bug in the LCL)

> We also have to remember that *probably almost nobody* remembers 
> fixing a bug related to this. That's because most people who come up 
> against this bug probably have no clue what is happening and only by 
> making some other changes do they coincidentally "fix" it. I propose 
> that for a problem like this you cannot rely on reported incidents to 
> determine how frequent it is.

I would gop as far as to say, every time someone use a "const string 
param" without knowing the real meaning,  the resulting code should be 
considder buggy. Even if it works by accident. Wrongly written code, 
that works by accident, to me is close enough to being a bug.

Looking at it like this, it is probably correct to say, that this 
"feature" has cause a lot of bugs already.

But it doesn't mean the feature itself is buggy. It maybe that it isn't 
documented well enough, as to few people are aware of it. It may even be 
that the indrotuction of this feature was not a good idea, or should at 
least have been done in a way that would make the "risks" more obvious.
Of course "should have done" is past, little point in discussing that now.

BTW, there are similar pitfalls with functions like fillchar, or move

x: array of string;
fillbyte(x[0], (length(x)-1)*sizeof(string), 0)

If there are any strings with content in the array, it will leak
using "move()" on an array like this will lead to crashes later in the 
code...

Only difference, for those functions more (not all) people know of the 
dangers...


This is one of the reasons, why I have asked for a range-ceck like 
feature for such const params (http://bugs.freepascal.org/view.php?id=19605)
- Not only would it help finding where the problem occurs
- It would also help creating awareness:
   Many people tend to switch on all the check options (range, overflow, 
io, stack...) during development. So would they do for a check detecting 
"const param" issues.
   The hope is that simple by noting the features, some would look up 
it's documentations. The others would do so, once the check raises an alert.