Reading
http://forum.lazarus.freepascal.org/index.php/topic,28740.msg180163.html#msg180163 and http://bugs.freepascal.org/view.php?id=28279

I did some tests and found something related, that I believe to be a bug. 2.6.4 and trunk (few weeks old)
Could someone please confirm?

The line

   s1 := copy(x,2,3)+'x';

creates a string with a refcount of 1 (for some reason if only using copy, but not +'x', then the ref count of S1 is 2)

The below outputs
0
0
0
-252645136

The 2nd call

   Foo2(s1,s1);

gets an invalid argument for "b"

The asm (trunk) is
.Ll26:
# [23] Foo2(s1,s1);
movl U_$P$PROJECT1_$$_S1,%ebx
movl $U_$P$PROJECT1_$$_S1,%eax
call fpc_ansistr_decr_ref
movl $U_$P$PROJECT1_$$_S1,%eax
movl %ebx,%edx
call P$PROJECT1_$$_FOO2$ANSISTRING$ANSISTRING
.Ll27:

ebx is a temp copy of s1, but then f1 becomes nil, and ebx points to freed memory.

program Project1;
procedure Foo1(a: AnsiString; out b: AnsiString);
begin
   WriteLn(length(a));  WriteLn(length(b));
   b := 'a';
end;
procedure Foo2(out a: AnsiString; b: AnsiString);
begin
   WriteLn(length(a));  WriteLn(length(b));
   b := 'a';
end;
const x: AnsiString = 'abcde';
var s1: AnsiString;
begin
   s1 := copy(x,2,3)+'x';
   Foo1(s1,s1);
   s1 := copy(x,2,3)+'x';
   Foo2(s1,s1);
   ReadLn;
end.

The answer is simple: Don't pass a variable you pass as an out-parameter also as a by-value or - even worse - const parameter. This is by design and an error in /your/ code.

Regards,
Sven

It is similar to

type
  tarray = array [1..10] of longint;
procedure p(const a1 : tarray;var a2 : tarray);
  begin
    a2[1]:=4321;
    writeln(a1[1]);  // surprise
  end;
var
  arr : tarray;
begin
  arr[1]:=1234;
  p(arr,arr);
end.

The only way to prevent this (of course, such a simple case could be detected by the compiler but one can always construct an example which works around this detection), is to turn off the const optimization that const allows the compiler to pass only a reference, but I am pretty sure, people will not be happy about this :)

Martin Frb wrote

ebx is a temp copy of s1, but then f1 becomes nil, and ebx points to freed memory.

It's because "out" finalises on the caller side (due to its COM origin, I guess) while value parameters get their reference count increased on the callee side (for efficiency reasons, I guess). This is all Delphi-compatible.

Jonas

Ok. that's why I asked before deciding if reporting.

I knew that the value of s1 (as in empty versus the last strings content) was more or less unpredictable.

But I thought that ref-counting was save, except for "const". This is why I asked.

I dont see any similarity. You pass 2 pointers to the same memory. No surprise for me. But also no broken ref counting.

Florian Klämpfl wrote

The only way to prevent this (of course, such a simple case could be detected by the compiler but one can always construct an example which works around this detection), is to turn off the const optimization that const allows the compiler to pass only a reference, but I am pretty sure, people will not be happy about this :)

Out of curiosity, how would you disguise it from the compiler? Actually, how would you do that in the example I posted. In your example it is not that hard to discuise (but then in your example there is nothing unexpected)

But in my example it is about refcounting (and pointing to unallocated mem)

   //procedure Foo2(out a: AnsiString; b: AnsiString);
   Foo2(s1,s1);

Foo2 is called with
1) the same string for both param
2) That string has a ref count of 1 before the call

How to you disguise it, without incrementing the ref count) If the refcount goes up, then all is ok, both arguments will be valid (that is either nil, or some string in allocated memory).

Also the behaviour is not documented.
http://www.freepascal.org/docs-html/ref/refsu62.html#x165-17500014.4.3

"conts" param
http://www.freepascal.org/docs-html/ref/refsu63.html#x166-17600014.4.4 explicitly document that ref counting is not done. (But I have no const in my example)

But "out" param have no such documentation. They also do *not* state in the doc that:
1) the ref count is decremented
2) May be decremented in a way that leaves other (ref counted) parameters in a zombie state (pointing to freed memory)

In fact the doc states

Yet "var" param also do not decrease the reference, so the statement is wrong/incomplete.

I dont have Delphi, but
http://forum.lazarus.freepascal.org/index.php/topic,28740.msg180201.html#msg180201

says Delphi handles it different.

Martin Frb wrote

But "out" param have no such documentation.

Also see the thread "Reference counting interface objects" from October 2014 and PR 26874 <http://bugs.freepascal.org/view.php?id=26874>

Regards,

Adriaan van Os

Well. First off, just so we can be clear. Things can be tested rather easily:

araminta: >cat trf.pp
{$mode objfpc}{$h+}
uses sysutils;
procedure DoByValue(S : String);
begin
   Writeln('By value : ',StringRefCount(S));
end;
procedure DoConst(const S : String);
begin
   Writeln('As Const : ',StringRefCount(S));
end;
procedure DoVar(var S : String);
begin
   Writeln('By ref : ',StringRefCount(S));
end;
procedure DoOut(out S : String);
begin
   Writeln('Out : ',StringRefCount(S));
end;
Var
   S1,S2 : String;
begin
   S1:=IntToStr(123)+'abc';
   S2:=S1;
   Writeln('Initial ref count : ',StringRefCount(S2));
   DoByValue(S2);
   DoConst(S2);
   DoVar(S2);
   DoOut(S2);
end.

araminta: >cat trf.pp
araminta: >./trf
Initial ref count : 2
By value : 3
As Const : 2
By ref : 2
Out : 0

Note the 0 !

Secondly, the section about 'out' parameters is very old; there were not nearly so much managed types at the time. I will update it, I was just documenting the new string types anyway. The above will nicely illustrate the point.

Michael.

What about the (unverified) remark on the forum that delphi does it different (apparently passing nil in that case)?

"const" is an exception. The purpose of const is to skip the ref counting.

"Out" has a different purpose. Should it really create a dangling pointer in a ref counted variable?

In your example you are lucky you got a refcount of 0. The memory is after-all freed. you called the StringRefCount on a dangling pointer. Anything could have happen.

Adding an additional indirection e.g. by a pointer, a class or an array.

Foo2(a1[i],a2[j])

a1, a2 being dyn. arrays, but sharing data, i and j having the same value.

var p1, p2 : pansistring

Foo2(p1^,p2^);

p1 and p2 pointing to the same data

etc.

Well, similar in the meta sense: passing the same data location with a different modifier.

No, the compiler *must* initialize it.
Without that guarantee, ansistring out parameters could never work.

If the out got a dangling pointer as input, it would never be able to decrease the ref. count when assigning something to it. Ergo, the compiler must initialize out params.

Michael.

Yes you are right. But that makes you example different. the value of the "out" param passed in is nil (because of the initialization). So the refcount of 0 is no my example the out param is fine too, but the other param (that gets passed the same param on the callee side) is dangling.

Normally only if the initial reference count was 1.

And now you know why reference counts can be dangerous and bite you in the lef:-)

But I will see if I can document the case.

Michael.

I dug out my old "turbo delphi".

In all both cases (my original example / you dyn array example) Delphi passes nil for the normal parameter, and fpc passes a dangling pointer.

Turns out it was already documented, but not yet on the website. There is a section devoted to managed types and reference counts.

I added my sample program, will see about concocting a simple version of yours.

Michael.

This depends on the order in which parameters are loaded and is coincidence. Nevertheless, I'll fix the evaluation order because the code is non-optimal (but not wrong!).

3.1.1 i386:
# [22] Foo2(s1,s1);
movl U_$P$PROJECT1_$$_S1,%ebx
movl $U_$P$PROJECT1_$$_S1,%eax
call fpc_ansistr_decr_ref
movl $U_$P$PROJECT1_$$_S1,%eax
movl %ebx,%edx
call P$PROJECT1_$$_FOO2$ANSISTRING$ANSISTRING

2.6.4 i386:
# [19] Foo1(s1,s1);
movl $U_P$PROJECT1_S1,%eax
call FPC_ANSISTR_DECR_REF
movl $U_P$PROJECT1_S1,%edx
movl U_P$PROJECT1_S1,%eax
call P$PROJECT1_FOO1$ANSISTRING$ANSISTRING

2.6.4 x86-64:
# [22] Foo2(s1,s1);
movq U_P$PROJECT1_S1(%rip),%rbx
leaq U_P$PROJECT1_S1(%rip),%rcx
call FPC_ANSISTR_DECR_REF
leaq U_P$PROJECT1_S1(%rip),%rcx
movq %rbx,%rdx
call P$PROJECT1_FOO2$ANSISTRING$ANSISTRING

I did all my tests with both orders
procedure Foo1(a: AnsiString; out b: AnsiString);
procedure Foo2(out a: AnsiString; b: AnsiString);

So in Delphi it does not depend on this order. The "none out" is always nil.

It is a decision of the code generator, the order of the parameters in the declaration does not matter.

procedure Foo1(a: AnsiString; out b: AnsiString); procedure Foo2(out a: AnsiString; b: AnsiString); So in Delphi it does not depend on this order. The "none out" is always nil.

Yes. Implementation specific behavior.

Really. Then Delphi documented that this (can) break ref counting?

Because if not, then the parameters are strings, which (except for const param) are properly ref counted, and can not become dangling pointers.

So unless Delphi documented that "out" breaks ref counting, then I consider this as documented behaviour.

To clarify:
* I consider "not passing a dangling pointer in a ref counted type)" as documented behaviour.
* passing "nil" may well be implementation specific. It would be equally god if the string previously in the variable would be passed, or if the choice would depend on other factors (may that be surrounding code, optimization level, compiler settings or whatever)

No idea. But not every border case needs to be documented. But I could imagine that it is possible to construct a case which breaks with delphi as well. Possible candidate:

procedure Foo2(out a: AnsiString; b: AnsiString);cdecl;

Or is it documented somewhere that out parameters are finalized first and then the other parameters are loaded?

Florian Klämpfl wrote

Or is it documented somewhere that out parameters are finalized first and then the other parameters are loaded?

Regardless of whether it's documented, maybe we should do that anyway. It doesn't seem like a bad idea.

Jonas

It would in any case not break Delphi Compatibility. And it would remove one pitfall for dangling pointer vulnerabilities.

A few observations I made. But they are mostly implementation detail, so if done other ways may be taken...

FPC currently simply decrements the refcount (on calling site). So if refcount > 1 then the string remains valid. (vaild = not a dangling pointer) If I am right a ref count of 1 can only happen, if the value is read directly from memory (variable, via pointer, property without getter)

function bar(a: AnsiString): AnsiString;
begin
   result := a;
end;
Foo2(s1,Bar(s1));
in the debugger shows the refcount goes up. So function results may be
safe anyway? If so eval orders of functions does not even need to change.

In Delphi I observed:
- The ref count does not matter. The other param (that has the same variable) always gets nil
- Except, if resut of a function call

   Foo1(Bar(s1),s1);
   Foo2(s1,Bar(s1));
   Bar always gets the actual value of s1, and then the result of Bar is

passed. (But again that really may be implementation detail)

Correct. Would definitely not be happy about this. :)

- Dennis