I noticed an incorrect handling of string parameters in a few RTS routines which might cause incorrect results when reading from strings where the actual parameters are expressions (not simple variables).
`ReadStr' and `Val' might be affected, though I haven't been able to reproduce an actual problem. The rarely used `StringTFDD_Reset' is affected, that's where I found the problem. (fjf1099.pas)
The attached patch fixes the bugs. For `StringTFDD_Reset', this results in an interface change, though for most uses it should be compatible (string -> conformant array of char), as long as the actual parameter is not an empty string.
You'll have to rebuild the RTS and your units. You can do `make pascal.update-release' in the build gcc subdirectory before making GPC, to force GPC to recompile things. (I'm not including a version change in the patch, as the patch might be useable against several releases this way, though I've tested it only against one.)
Waldek, you told me you're going to release a new snapshot soon. Please test this patch with your current code and include it if possible.
Frank
Frank Heckenbach wrote:
I noticed an incorrect handling of string parameters in a few RTS routines which might cause incorrect results when reading from strings where the actual parameters are expressions (not simple variables).
`ReadStr' and `Val' might be affected, though I haven't been able to reproduce an actual problem. The rarely used `StringTFDD_Reset' is affected, that's where I found the problem. (fjf1099.pas)
It looks that the problem is deeper. The following program fails both with and without your patch:
program rstr1(Output); type ii = integer value 0; tip = ^ii; var ipv1 : tip; iv2 : integer; s : string(20);
function ip1: tip; var tmp : tip; begin s := 'dead beef'; ip1 := ipv1; end; begin s:='666 123'; new(ipv1); readstr(s, ip1^, iv2); if (ipv1^ = 666) and (iv2 = 123) then writeln('OK') end .
AFAICS if the first parameter to `readstr' is not a plain variable, or if any other parameter may have side effects, then we need to copy the string parameter. When passing string expressions as const parameters we create a temparary variable, but is is not clear if the temparary will live long enough (in fjf1099.pas the temporary is destroyed too early, but I do not see why other similar cases work).
Frank Heckenbach wrote:
I noticed an incorrect handling of string parameters in a few RTS routines which might cause incorrect results when reading from strings where the actual parameters are expressions (not simple variables).
`ReadStr' and `Val' might be affected, though I haven't been able to reproduce an actual problem. The rarely used `StringTFDD_Reset' is affected, that's where I found the problem. (fjf1099.pas)
As I wrote, for `ReadStr' to right thing to do is to copy the argument (this is what the standard prescribes). However, I do not know what `StringTFDD_Reset' is supposed to do. ATM it looks that `StringTFDD_Reset' assume that the arguments remain valid when reading from the corresponding file. So, the natural specification can be: "Arguments to StringTFDD_Reset must be variables, must stay unmodified as long as ....". This specification is messy and the usage is error prone, but with this specification the fix would be to change the third parameter to StringTFDD_Reset into a var parameter. OTOH if we want to promise more, then we need to think twice how we can implement this.
Waldek Hebisch wrote:
It looks that the problem is deeper. The following program fails both with and without your patch:
I see. (BTW, I think it's actually a different bug than the one I fixed, but it doesn't matter.)
AFAICS if the first parameter to `readstr' is not a plain variable,
In this case I think we only need to fix the reference, which save_expr_string already does. (E.g., functions returning strings use a temporary variable internally, and save_expr_string makes sure that the function is called only once, but the internal variable can be used several times.)
or if any other parameter may have side effects, then we need to copy the string parameter.
Yes. We should do a new_string_by_model then, I think. (The check for side-effects could be refined, of course, but I think it would already catch the majority of cases.)
For `Val', we either have to check the 2nd argument for side-effects and copy the string if so, or (perhaps better), force evaluation of the conversion (whose result is always a simple numeric type) before evaluation of the 2nd argument via a save_expr or temp variable. (I'm not sure if BP actually documents this as the standards do for `ReadStr', but it seems to behave like this.)
When passing string expressions as const parameters we create a temparary variable, but is is not clear if the temparary will live long enough
That's what I avoided with my patch.
(in fjf1099.pas the temporary is destroyed too early, but I do not see why other similar cases work).
I'm also not sure why other cases seemed to work, but I really didn't try too hard to break them as I already knew it was wrong.
However, I do not know what `StringTFDD_Reset' is supposed to do. ATM it looks that `StringTFDD_Reset' assume that the arguments remain valid when reading from the corresponding file. So, the natural specification can be: "Arguments to StringTFDD_Reset must be variables, must stay unmodified as long as ....". This specification is messy and the usage is error prone,
IMHO it's more or less analogous with files. One could argue about what happens when the length changes, but on files with read-buffering (which GPC does), reading can also be undefined when the external file is modified meanwhile.
but with this specification the fix would be to change the third parameter to StringTFDD_Reset into a var parameter.
That's why I did so.
OTOH if we want to promise more, then we need to think twice how we can implement this.
I'm not sure we should. The user can always make a more permanent copy himself when needed.
Frank
Frank Heckenbach wrote:
Waldek Hebisch wrote:
<snip>
but with this specification the fix would be to change the third parameter to StringTFDD_Reset into a var parameter.
That's why I did so.
I mean that to fix StringTFDD_Reset the rest of the patch is not needed. And I just checked that full patch caused regression in fjf629e.pas and fjf629g.pas
Waldek Hebisch wrote:
Frank Heckenbach wrote:
Waldek Hebisch wrote:
<snip> > > but with this specification the fix > > would be to change the third parameter to StringTFDD_Reset into a var > > parameter. > > That's why I did so.
I mean that to fix StringTFDD_Reset the rest of the patch is not needed.
Perhaps the previous code was not strictly wrong, but it was quite fragile. It relied on a temp copy made during parameter conversion for the internal call to ReadStr_Init to last over subsequent calls. (Besides, the new code might produce somewhat more efficient code, as it avoids a few unnecessary copies. As you said, the other bug, where a necessary copy isn't made, is independent of my patch.)
And I just checked that full patch caused regression in fjf629e.pas and fjf629g.pas
The patch shouldn't change anything about FormatString (except a detail for CString parameters, which don't occur here).
I also ran the tests, and it didn't fail (with my current code and with 20051116). Perhaps it interfered with your recent changes WRT StringOf? Otherwise it seems to be a somewhat volatile bug, but I don't yet see it. Which regressions do you get?
... Did you rebuild the RTS as I wrote in my original mail? I now did some experiments -- indeed, if I build 20051116 without the patch, then apply the patch, make again without rebuilding the RTS, I get those failures. But these are then simply due to an inconsistent compiler/RTS, I suppose.
Frank
Frank Heckenbach wrote:
Waldek Hebisch wrote:
Frank Heckenbach wrote:
Waldek Hebisch wrote:
<snip> > > but with this specification the fix > > would be to change the third parameter to StringTFDD_Reset into a var > > parameter. > > That's why I did so.
I mean that to fix StringTFDD_Reset the rest of the patch is not needed.
Perhaps the previous code was not strictly wrong, but it was quite fragile. It relied on a temp copy made during parameter conversion for the internal call to ReadStr_Init to last over subsequent calls. (Besides, the new code might produce somewhat more efficient code, as it avoids a few unnecessary copies. As you said, the other bug, where a necessary copy isn't made, is independent of my patch.)
The change looks like a good one.
And I just checked that full patch caused regression in fjf629e.pas and fjf629g.pas
The patch shouldn't change anything about FormatString (except a detail for CString parameters, which don't occur here).
I also ran the tests, and it didn't fail (with my current code and with 20051116). Perhaps it interfered with your recent changes WRT StringOf? Otherwise it seems to be a somewhat volatile bug, but I don't yet see it. Which regressions do you get?
... Did you rebuild the RTS as I wrote in my original mail? I now did some experiments -- indeed, if I build 20051116 without the patch, then apply the patch, make again without rebuilding the RTS, I get those failures. But these are then simply due to an inconsistent compiler/RTS, I suppose.
My fault: rebuilding RTS fixed the problems.
-
Frank Heckenbach -
Waldek Hebisch