Classic compiler (Win32) strings double-free crash because of error in reference-counting.
Classic compiler (Win32) strings double-free crash because of error in reference-counting.
Tested with XE2 and 10.1 Berlin
Test applications:
https://github.com/the-Arioch/XE2_AutoOpenUnit/tree/master/Delphi_String_Bug
UPD's Girli's version in that folder is probably THE ultimate "minimal reproducible example" without need for external files.: https://github.com/the-Arioch/XE2_AutoOpenUnit/blob/master/Delphi_String_Bug/Girli_str_2xFree_Minimized.dpr
The initial repro requirement - the project should find its dproj-file and then its dpr-file to run the bug triggering code path.
Discussions, minimizations and testing: http://www.sql.ru/forum/1300873-a/xe2-string-double-free
https://github.com/the-Arioch/XE2_AutoOpenUnit/blob/master/Test/xe2_str_2xFree.dpr
Tested with XE2 and 10.1 Berlin
Test applications:
https://github.com/the-Arioch/XE2_AutoOpenUnit/tree/master/Delphi_String_Bug
UPD's Girli's version in that folder is probably THE ultimate "minimal reproducible example" without need for external files.: https://github.com/the-Arioch/XE2_AutoOpenUnit/blob/master/Delphi_String_Bug/Girli_str_2xFree_Minimized.dpr
The initial repro requirement - the project should find its dproj-file and then its dpr-file to run the bug triggering code path.
Discussions, minimizations and testing: http://www.sql.ru/forum/1300873-a/xe2-string-double-free
https://github.com/the-Arioch/XE2_AutoOpenUnit/blob/master/Test/xe2_str_2xFree.dpr
Be carefull when you use the const string parameters. Delphi does not increment the count of references for them. If a circular reference is possible, the fix:
ReplyDeleteprocedure Test(const ConstFN: string); // Hulk crash!
var
fn: string;
begin
fn := ConstFN;
VolatileFN := Copy(ConstFN, 1, 12);
TFile.ReadAllText(fn);
end;
If you will create a new RSP I've simplified demo for it:
https://pastebin.com/swc5jk8S
T n T after QC closed and Jira was made non-public i did not bother with getting login for it. I am still using XE2 afterall.
ReplyDeleteIf you want, open the ticket, just give credits to me and to that another SQL.ru guy.
PS. I know about const and ref-counting optimization. Just Delphi should pay attention when the string is really const-only and when it is const-shared. In particukar that fix of yours was thing i did in the real project and is the #1 workaround in the xe2 test app.
I also suspect there are two different bugs, as the one reported for 10.2 is not reproduced in xe2.
That is why i want you to keep comments with clear description of the what bug was reproduced in which Delphi versions
There is one more bug, destroying data in array of string constants. But i feel too lazy to make that rather complex test groupproj :D
Arioch The I can't see other issues in your test projects. I tested it in 10.2.3. Is it a bug or a feature (an optimization)? I know about this special behavior of Delphi strings for a long time, and I know how to bypass it.
ReplyDeleteTo make more precise code optimization, the compiler must deeply analyze not only the syntax, but also the semantics of the source code. It's not easy to implement.
P.S. I've a personal license for XE2 too, but I've uninstalled it when CE was released. Delphi Community Edition is very good, I'd like it so much. The dark theme is great for working at night. Highly recommended.
For me Delphi CE woukd be just a pirated Pro, why bother.
ReplyDeleteThe "simplified demo" did not trigger the bug in XE2, so it most probably another, new bug introduced later than XE2.
Yes, it definitely is the bug in the compiler, when the correct code does destroy the data.
Arioch The If you use XE2 for serious commercial projects then yes, CE is not for you.
ReplyDeleteThe "error" is still there in my demo, even if XE2 does not report it.
While syntactically OK, the code semantically is incorrect. But the compiler can not see this, it's the problem.
They can increment the reference count for every const string parameter, and add the finalization section (the hidden try-finally block) for every procedure and function with such parameters, but do we really need it?
T n T it does not matter if i use xe2 or not, what matters is salary ("total revenue") $5K a year and above
ReplyDeleteT n T what do you mean by "report it" ?
ReplyDeleteArioch The Your salary can be $100500, but you still can use CE for amateur projects at home.
ReplyDeleteXE2 silently use already freed string in the demo, without a runtime error. It depends of TFile.ReadAllText realisation in XE2. I can demonstrate the same error without TFile:
function Add1(const s: string): string;
begin
Result := s + '1';
end;
procedure Test(const ConstFN: string); // Hulk crash!
//*var
//* fn: string;
begin
//** fn := ConstFN;
VolatileFN := Copy(ConstFN, 1, 12);
//** Add1(fn);
Add1(ConstFN);
end;
This code raise the Out of memory exception on CE, while a semantic error is the same.
T n T no, EULA is quite clear on it and bothers to several times repeat that what matter is total revenue, both related and unrelated to Delphi. Thus even for home use it would be just a comfy pirated Pro.
ReplyDeleteIn my initial demo, derived from a real small project, XE2 run into Invalid Pointer Operation, calling FreeMem twice for the same pointer.
While use after free is a bug bad enough, it is different from double-free bug.
Also, i do not think that 10.2 started by default wiping out freed memory, thus this alone would not had caused errors in both version, thus some another bug introduced after XE2.
I'll check this code when have time. Semantically the code is correct. Making OOM because of using garbage instead of variable is not.
Actually this your last code, Delphi implementation wise is the same as
ReplyDeleteprocedure Add1(const s: string; var Result: string);
begin
Result := s + '1';
end;
procedure Test(const ConstFN: string); // Hulk crash!
var
fn: string; // "invisible" unnamed local var originally
begin
VolatileFN := Copy(ConstFN, 1, 12);
Add1(ConstFN, fn);
end;
Frankly, here is nothing complex to confuse the compiler.
fn should be zeroed out before passing to Add1
Add1 should not be changing ref-counts of both.
The string+char intrinsic routine should copy string content without modifying source string.
Imagine two routines:
procedure _RTL_StrAddChar_Ternary(const src: string; const add: char; var Result: string);
procedure _RTL_StrAddChar_InPlace(var SourceResult: string; const add: char);
procedure Add1_a(const s: string; var Result: string);
begin
// Result := s + '1';
_RTL_StrAddChar_Ternary(s, '1', Result);
end;
procedure Add1_b(const s: string; var Result: string);
begin
// Result := s + '1';
Result := s;
_RTL_StrAddChar_InPlace(Result, '1');
end;
procedure Add1_c(const s: string; var Result: string);
begin
// Result := s + '1';
_RTL_StrAddChar_InPlace(s, '1');
Result := s;
end;
We may argue which implementation, A or B, can be in average more efficient.
But implementation C breaks the contract and should never be allowed.
There is no need for the compiler to make some uber-complex analysis, it should just follow declarations when chosing intrinsics.
Here is a snippet:
ReplyDelete"Am I Eligible for Delphi Community Edition?
If you're an individual you may use Delphi Community Edition to create apps for your own use and apps that you can sell until your revenue reaches $5,000 per year."
That statement refers to revenues from any apps that you create with CE, not revenues from other sources.
Arioch The procedure Add1_c will not compile because E2197 Constant object cannot be passed as var parameter.
ReplyDeleteTo prevent the semantic error in your original dpr the compiler should refuse to compile the line
VolatileFN := copy(ConstFN, 1, 12);
because somewhere in the call stack VolatileFN variable was passed as const parameter to a routine. Or alternatively the compiler should refuse to pass VolatileFN variable as const parameter to a procedure because somewhere in the subroutines this variable will be modified. It's not a simple task.
Jennifer Powell yes, the "cumulative revenew" for all your activities, both related and unrelated to programming, related ones both programmed with Delphi or not. In the CE announcement thread i quoted quite a few snippets.
ReplyDeleteT n T static analisis of semantics and execution paths would not do, not alone at least, for there can be multithreaded apps...
ReplyDeleteT n T in general mixing memory models is a gotcha even with different types, like TXMLDocument shows.
ReplyDeleteHowever mixing different models within one type is yet worse. I see the only correct approach would be to recognize const and volatile strings for different types they are and to have clearly defined transition rules.
Under those rules const to const assignment may do without ARC, but volatile to const should uptick. Anything to volatile does uptick already
T n T here is another minimization, and that is probably the ultimate one: https://github.com/the-Arioch/XE2_AutoOpenUnit/blob/master/Delphi_String_Bug/Girli_str_2xFree_Minimized.dpr
ReplyDeleteArioch The If you are using Delphi to develop apps for your business and the business revenue is more than 5K, CE is not for you even if you do not have any revenue from the apps.
ReplyDeleteIf you are multi-millionare business owner that uses Delphi at home for fun and have under 5K revenue from the apps you develop, you can use Delphi CE.
You've managed to create 2 string variables referencing the same string with refcount=1. This is compiler bug already, the compiler should not allow to do it without hacking internal string structure; after you copied one variable to another you created string with refcount=0, and this is the cause of invalid pointer exception.
ReplyDeletePS: Refcounting bugs are serious bugs, and you'd better register and submit bug report.
Sergey Kasandrov IMHO this is not a bug, but the lack of semantic analysis in the compiler. May be TMS Software can add detection of such situations in FixInsight? Would be great. Roman Yankovsky
ReplyDeleteT n T No, it is 100% compiler refcounting bug, and IMHO serious compiler bug.
ReplyDeleteSergey Kasandrov Delphi should refuse to compile a semantically incorrect source code, but it compiles. If this is a bug, then so be it.
ReplyDeletejeff weir You have to be a little bit careful with your last example.
ReplyDeleteIf you are a business owner and operate under an assumed name, for example in the USA with what is referred to as a dba, then you are a legally a company as an individual, sole proprietor as it is called. You would then not be eligible to use CE unless you were a statup with no company or personal revenue, which in both cases are legally considered the same in the USA via the dba.
T n T References to compiler-managed instances should ALWAYS be counted correctly by the compiler, even if the code does not compile :) . The OP example shows that the compiler can do incorrect refcounting.
ReplyDeleteSergey Kasandrov Why should the compiler increment the reference count for constant parameters of subroutines? They should not change if the program is well thought out. If your code is semantically correct, you will not encounter such an error as in the code demonstrated by Arioch The.
ReplyDeleteLook at FixInsight page. The code:
try
f := TForm.Create(Self);
f.ShowModal;
finally
f.Free;
end;
While this code is syntactically correct and can be compiled, it can lead to hard-to-find run-time errors. But should we reproach Delphi that it did not catch this error at compile time, and call it a compiler error?
T n T Compiler should not increment refcount for constant parameters. I don't want to debug the code more to see what exactly is wrong, this not my job; I just see that compiler counts references incorrectly; if this is result of some optimization, IMO the optimization should either fixed or disabled.
ReplyDeleteSergey Kasandrov My pet programs actively use Delphi strings inside (text processing and translations). I would not want to get a noticeable impact on performance due to the disabling of some optimizations. It would be better if the compiler showed a warning in such cases.
ReplyDeleteT n T
ReplyDeletetry
f := TForm.Create(Self);
this code is clear "use of uninitialized variable" in the finally-part and is well worth warning.
The initial code with string variables is not so, it is semantically correct, it is just the compiler that looses track of values when compiling it
"Why should the compiler increment the reference count for constant parameters of subroutines? "
ReplyDeleteIt does not "should", it can choose any other strategy to correctly count its own references.
"If your code is semantically correct" - and it is. I do not ask compiler to count references. I do not ask compiler to free memory. I ask compiler to pass string value into the procedure - and it should do just that.
If the compiler made its own decision to count references or free memory - it is compiler's decision not mine, and it is compiler who bears responsibility for that
Final note: the const parameters of string type, as well as other reference counted types are specially optimized for speed. If you need to change a variable declared in the parent scope that was passed as const parameter, or this variable can be changed in another thread - you won't use const.
ReplyDeleteSome articles to read:
delphitools.info - All hail the "const" parameters! - DelphiTools
https://stackoverflow.com/questions/5851584/are-const-string-parameters-thread-safe (see the answer of Barry Kelly, the former Delphi compiler engineer)
https://stackoverflow.com/questions/11001477/what-difference-does-it-make-when-i-use-const-in-a-procedures-parameter
Please file a QP report. The first step to getting any bug fixed is filing a bug report.
ReplyDelete"I ask compiler to pass string value into the procedure - and it should do just that."
ReplyDeleteArioch The No you asked exactly the opposite and compiler listened to your request.
There is error in code, not the compiler. Yes, I know you can perfectly shoot your self in the foot with reference counting, but it is not the ARC fault. It does what it does and it has its limits. It cannot detect broken code.
David Millington like i just said above, i can't do it, but anyone of you is welcomed to
ReplyDeleteDalija Prasnikar no i did not. I did not asked compiler to use ARC at basic datatypes afteral. OLE does not and has much more footprint that Delphi ever had. Yet less so did i ask it to use broken ARC.
ReplyDeleteThere is no broken code, there just is compiler that can't can tell its own left foot from right
T n T it seems exactly the same "optimization" as their decision to skip detecting exceptions in LLVM try blocks until developer after reading fineprint would add extra commands to ask compiler to start catching exceptions inside this particular try block. Delphi was always proud how fast their try blocks work, they even issued some patents on it, so they keep optimizing.
ReplyDeleteT n T thanks for SO links a lot
ReplyDeleteYou are welcome.
ReplyDeleteAfter the release of 10.3 everyone will have the opportunity to create their own string type, based on improved record type. Lightweight and fast, or heavy and safe, with or without the reference counting, with or without the codepage, nullable? - you choose.
Well, if i would bother purchasing 10.3, hardly so.
ReplyDeleteAnd hardly it would give access to compiler intrinsics like Nemerle and Scala do anyway.
At least people would get warned, if they somehow would get landed on those page before hitting the trap. Hens and eggs problem, as usual :)
Arioch The I see my comment was easy to misinterpret.
ReplyDeleteI commented merely on passing value in relation to const parameter. Too many times people blame ARC for their mistakes.
Maybe there is a compiler bug lurking in that code - I didn't go into full analysis. It certainly would not be the first one nor the last one. Even if it is a bug it is just a compiler bug. ARC or no ARC.
BTW, you can use Quality Portal. Just use your EDN login username (not email) and password and you should be able to log in and report bugs.
No compiler warnings about passing const params to other functions.
ReplyDeletehttp://docwiki.embarcadero.com/RADStudio/Berlin/en/String_Types_%28Delphi%29
No big red letters warning about data destruction, no any warning actually.
http://docwiki.embarcadero.com/RADStudio/Berlin/en/Parameters_(Delphi)#String_Parameters
>>Because S1 and S2 are not modified in the body of CompareStr, they can be declared as constant parameters.<<
Encoraging, without warning.
>>String Parameters<<
The whole paragraph on short string, not a single mention of RC-strings.
Arioch The I am not sure what you mean. Why would there be any warnings?
ReplyDeleteDalija Prasnikar because we have a situation where compiler violates basic language rules and destroys developer's data, because of its limitations and implementation details. Of course compiler should shout out about the risks of data destruction because rules would be violated. Relying upon implementation details is code smell, isn't it? So we by default should not do it and think about it. So the compiler should proactively inform us where code smell is required for apps and libs to work reliably.
ReplyDeleteArioch The Again, I didn't made deep analysis of code in question.
ReplyDeleteIn theory there are two possible outcomes with this or any other code: either there is a bug in your code (or RTL code) or there is a bug in a compiler.
If there is a bug in your code (or RTL code) then it is because you (someone) wrote code that violates basic principles. Compiler can emit warnings in some cases, but it cannot give warning for each and every piece of broken code someone might write. If there is a bug in a compiler there is nothing to document or warn about. It is a bug.
Dalija Prasnikar Third outcome: as designed. )
ReplyDeleteT n T I would say that this falls under your code is wrong because it does not follow the rules (even if the rules are broken or bad) ;)
ReplyDeleteFWIW, we had an engineer look at this. It is a code error. To quote,
ReplyDelete"System.Copy() returns a unique string which deliberately overwrites the global variable used for the const parameter which in this instance is passed by reference.
Doing this, the original string is freed. Subsequent references to this space will correctly produce an EInvalidPointer exception.
In such a circumstance, the use of a const parameter is not appropriate. Dependence on a variable from an outer scope as seen in the sample program indicates that care must be taken before deciding against using a value parameter. for the function argument."
IMO this doesn't warrant a warning - it is simply incorrect code.
David Millington Yes, this is a semantic error in the code, as I indicated above. Still, it would be nice if the compiler or some auxiliary utility (FixInsight) would show a warning, so the programmer could avoid such a situation.
ReplyDelete