Lazarus 1.8.1 bugs

[timppl]

I found a couple of bugs in Lazarus 1.8.1 which caused a segmentation fault. I have noted them on Mantis ( with patches ) with numbers 0032785 and 0032786.

Regards

Tim

[zeljko]

hmm...I've looked into your patches and it does not look good. Are you saying that checking for assigned pointer is crashing ? IMO, there's another problem on your system.

[Thaddy]

Yes. both patches are complete nonsense. (Don't let that discourage you to provide patches, though)
The assignment tests are strictly necessary so can not be removed.
You should test if any variables are initialized in the first place if there are issues (Default() intrinsic?), not mask out or ignore any manifestations.
Objects and classes are guaranteed to be initialized correctly.

I agree with zeljko : there must be something wrong on your system, code or installation.

Maybe you have a case of use-after-free?

[Martin_fr]

It may be possible that the patches are simply reverse.

I checked codeexplorer.pas and it already does not have the "assigned" that the patch wants to remove.

Maybe the diff was done in the wrong direction?

That said, it be good to know, how to reproduce the issues.
After all the patch may stop the crash. But since the author of the code did not expect that, subsequent code may still display something wrong.

[JuhaManninen]

The issue #32785 is about Delphi converter. I assume the patch is reverted but I still doubt it is needed.
There are no steps to reproduce, no example file to convert. Nothing. Not a very good report.

[timppl]

Hi guys

Thanks for the lack of appreciation . I produced the patches using svn so it is possible they are reversed. The problem is real in that assumptions are made that allocations always succeed and sometimes it doesn't, and in both cases lazarus was trying to use code that has not been allocated.
I do not have time to try and provide a small test case as this debugging was done taking time out of my paid job. The problem(s) occurred whenever I started lazarus 1.8.1 but not with 1.6.4, which suggests that  the problem is in the lazarus code

@Juha I could not attach the file that caused the crash as it is proprietary code.

Tim

[rvk]

Tim, don't see it as a lack of appreciation. Bug reports are always appreciated. But applying your fixes will just cover up another, maybe even bigger bug/issue.

PS. The fixes do seem to be reversed (as in the assigned should be added instead of removed).

The reason for a bigger issue is that the only place where I can see procedure CommentAutomatic is used is in convertdelphi.pas. In there, the calling of CommentAutomatic(fAllCommentedUnits) is done. But fAllCommentedUnits is always created in TConvertDelphiProjPack.Create(). So CommentAutomatic() could/should NEVER be called with an undefined fAllCommentedUnits.

Code: Pascal  [Select][+][-]

1. constructor TConvertDelphiProjPack.Create(const AFilename, ADescription: string);
2. begin
3.   //..
4.   fAllCommentedUnits:=TStringList.Create;
5.   fAllCommentedUnits.Sorted:=True;
6.   //..
7. end;

So, if there is a problem in your setup, somehow fAllCommentedUnits is freed or corrupted.
It's better to find out why (because this shows an issue elsewhere) than to fix it with not assigned(fAllCommentedUnits).

That's why applying these fixes may not be wise because they cover up your bigger issue.
(and we don't dispute you have an issue)

[timppl]

Thanks for that RVK.  I will try and find a bit of time to look into that tomorrow.

In the other patch there is a problem with the lazarus code. It is in 1.8.0 as well. In codeexplorer.pas line 1284 we have

Code: Pascal  [Select][+][-]

1.  
2. TVNode:=AddCodeNode(cefcLongProcs,ProcNode);
3. TVNode.Text:=TVNode.Text+' ['+IntToStr(LineCnt)+']';
4.  

In the function AddCodeNode called by this we have (codeexplorer.pas line 1170)

Code: Pascal  [Select][+][-]

1.     if ObsTVNode.Count>=CodeObserverMaxNodes then
2.     begin
3.       fObserverCatOverflow[f]:=true;
4.       exit(nil);
5.     end;
6.  

What is happening is that this function exits with return value nil but this is not checked for at line 1285 i.e TVNode  is assumed to be valid.

Regards

Tim

[JuhaManninen]

I produced the patches using svn so it is possible they are reversed.

AFAIK "svn diff" does not create reversed patches. I wonder how you created them.

Anyway, after the initial turnoff caused by invalid patches and missing steps to reproduce, I analysed the relevant code.
Both patches in their reverse form seem to be valid!
I applied the tests as suggested. See my notes in the related reports.
I don't understand how these issues have not caused crashes for others. Many people, including me, use the Code Explorer daily.
I have tested the converter many times, although more with full projects than single units.
Such is life...

Thanks.

[rvk]

Both patches (in their reverse form) seem to be valid!
I applied the tests as suggested. See my notes in the related reports.

I still think the converter/usedunits.pas fix will cover up some deeper problem.(which Tim should test for).
CommentAutomatic() couldn't be called with an unassigned parameter. So fixing this will cover that problem up.
The fix won't hurt otherwise.

The other fix for AddCodeNode is indeed correct.

[JuhaManninen]

I still think the converter/usedunits.pas fix will cover up some deeper problem.(which Tim should test for).
CommentAutomatic() couldn't be called with an unassigned parameter. So fixing this will cover that problem up.

Now CommentAutomatic() can be called with an unassigned parameter.
fOwnerConverter.fAllCommentedUnits can be Nil. This is a design choice and it allows reusing code when converting a single unit and a whole project / package. In essense fAllCommentedUnits was moved one level up in the class hierarchy although it is logically needed only in TConvertDelphiProjPack.
fOwnerConverter.fAllCommentedUnits is passed only to 2 methods: MoveMissingToComment which checked for Nil, and CommentAutomatic which didn't check for some reason.
I improved the code a little more in r56663.