Codeblocks crashing when closing project options dialog

[Jan van den Borst]

LS,
I have been having crashes when closing the project options dialog. Having some spare time available I started debugging and pinpointed the problem down to the file sdk\projectfile.cpp , svn rev 3664, line 281:

    Compiler* compiler = target
                            ? CompilerFactory::GetCompiler(target->GetCompilerID())
                            : CompilerFactory::GetDefaultCompiler();

The pointer is not checked against NULL. Therefore in my project file codeblocks  crashes at line 360:

fname.SetExt(compiler->GetSwitches().objectExtension);

I don't know if the pointer is to become NULL but in my case it does.
A simple:

 if (!compiler)
   return;

just after line 281 prevents the crash.

Kind regards
Jan

[thomas]

That seems to be right. Although there is a check for null-ness on that variable a few lines later, the check is not good (both branches use it).

[Biplab]

But I've got couple of questions.

1. Why doesn't the following code crashes?
Line 286: projectfile.cpp

Code

    if (target && ft == ftHeader && compiler && compiler->GetSwitches().supportsPCH)

2. Why compiler is getting NULL value? At least it should get a Compiler.

The worst case (where compiler variable would get NULL value)
  i) could be target is NULL and no default compiler is set.
  ii) GetCompiler() is returning 0 as it can't find a proper match.

[MortenMacFly]

1. Why doesn't the following code crashes?
Line 286: projectfile.cpp

Code

    if (target && ft == ftHeader && compiler && compiler->GetSwitches().supportsPCH)

Because if compiler is NULL all following checks are *not* done. The if statement exits early if the fist boolean is false... at least this applies to GCC. Other compiler *might* handle this different.

2. Why compiler is getting NULL value? At least it should get a Compiler.

Actually this can easily happen. Suppose you create a copy of a compiler and setup a project based on that. Now if you open the project on another PC where this compiler (the copy) isn't setup then the compiler will usually be NULL. Another option is that the one or other (like me ;-)) might have base compilers integrated that do not exist at all. In that case opening such a project would result in the compiler being NULL, too.
In the end there is another option: Disable the compiler plugin and the compiler will always be NULL. ;-)

BTW: You can easily simulate these cases by modifying a C::B project file and setting the compiler to an invalid string like: <Option compiler="does_not_exist" />... and/or disable the compiler plugin. This reminds me that at the time Yiannis implemented the plugin activate/de-activate facility we were hunting quite some bugs with missing checks for a valid compiler. Maybe we should repeat this step from time to time. ;-)

With regards, Morten.

[Biplab]

Thanks Morten for the clarification.

Because if compiler is NULL all following checks are *not* done. The if statement exits early if the fist boolean is false... at least this applies to GCC. Other compiler *might* handle this different.

I was doubtful about this answer. Thanks for confirming it.

2. Why compiler is getting NULL value? At least it should get a Compiler.

Actually this can easily happen. Suppose you create a copy of a compiler and setup a project based on that. Now if you open the project on another PC where this compiler (the copy) isn't setup then the compiler will usually be NULL. Another option is that the one or other (like me ;-)) might have base compilers integrated that do not exist at all. In that case opening such a project would result in the compiler being NULL, too.
In the end there is another option: Disable the compiler plugin and the compiler will always be NULL. ;-)

BTW: You can easily simulate these cases by modifying a C::B project file and setting the compiler to an invalid string like: <Option compiler="does_not_exist" />... and/or disable the compiler plugin. This reminds me that at the time Yiannis implemented the plugin activate/de-activate facility we were hunting quite some bugs with missing checks for a valid compiler. Maybe we should repeat this step from time to time. ;-)

So it is a problem of 2nd type as I mentioned. I think we should put a MessageBox throwing some *Warning* message before returning 0 in GetCompiler() or at least throw some messages in the Debug-Log. User would come to know about this problem then.

Alternatively it can be set to return Default Compiler.

Regards,

Biplab

[MortenMacFly]

I think we should put a MessageBox throwing some *Warning* message before returning 0 in GetCompiler() or at least throw some messages in the Debug-Log. User would come to know about this problem then.

I'm not sure about that. This may confuse people who disabled the compiler plugin on purpose (imagine they are using C::B for Python development or similar). Because they would see this message over and over again (just count the number of GetCompiler() calls accross C::B). An AnnoyingDialog wouldn't be of much help I suppose.

Alternatively it can be set to return Default Compiler.

This wouldn't work entirely if the compiler plugin is disabled because in that case there is no default compiler.

In the end I believe mainly we should ensure proper NULL checking and react appropriate in the calling method, thus not allow GetCompiler() to report "misuse". Why? Because GetCompiler() can't really know whether it *is* a misuse or not. The call can be fully correct and not an error.

With regards, Morten.

[MortenMacFly]

Ok... I've had another look into. I think to fix this issue we should simple edit lines 348- ~368 in projectfile.cpp as following:

Code

            else
            {
                if (pf->GetParentProject()->GetExtendedObjectNamesGeneration())
                {
                    object_file_native = objOut + sep + fname.GetFullPath();
                    object_file_flat_native = objOut + sep + fname.GetFullName();

                    if (compiler)
                    {
                        object_file_native += _T('.') + compiler->GetSwitches().objectExtension;
                        object_file_flat_native += _T('.') + compiler->GetSwitches().objectExtension;
                    }
                }
                else
                {
                    if (compiler)
                        fname.SetExt(compiler->GetSwitches().objectExtension);
                    object_file_native = objOut + sep + fname.GetFullPath();
                    object_file_flat_native = objOut + sep + fname.GetFullName();
                }
            }

And that's it. This will fix the crash. Unfortunately I don't have access to SVN ATM - maybe another dev could... after approval?!
With regards, Morten.

[Biplab]

That's a nice fix.

This will fix the crash. Unfortunately I don't have access to SVN ATM - maybe another dev could... after approval?!

I can put it, if you wish.

Regards,

Biplab

[MortenMacFly]

I can put it, if you wish.

I didn't verify completely but I believe things are better with this "patch" applied... so please go on! ;-)

[Biplab]

One more clarification: It would avoid the crash. But if the object extension is not set, will it compile? I think it can create problem. IMHO, user should be informed that C::B can't get compiler to avoid further problem.

Or, let's fix the crash first and we'll think about the other part later.

Waiting for your reply before I apply the patch.

Regards,

Biplab

[MortenMacFly]

But if the object extension is not set, will it compile?

No, but remember: If compiler is NULL then it *can't* compile because there is no (valid) compiler. ;-) So for this patch this is OK... but I agree -> there should be a message for the case the compiler is enabled but not valid. I'd say we have to ask Yiannis about that. But he'll surely say "Yes" to this "patch" (or throw a brick at me).
With regards, Morten.

[Biplab]

Poor question. It shouldn't compile.

Ok, I'm applying it.

Edit 1: Applied patch. In revision 3665. Thanks to Jan van den Borst for posting the bug.

[MortenMacFly]

Edit 1: Applied patch. In revision 3665. Thanks to Jan van den Borst for posting the bug.

Sorry for answering a little late (I've been quite busy today). Thanks for applying - unfortunately I'd say it only one half of the proposal (my fault - I should really have produced a true patch file). There is another crash candidate in the first if-clause. If I look at the diff in the WebSVN it seems this part is missing... I might be wrong, though... Could you have another look, please?
With regards, Morten.

[Biplab]

Yes Martin, you've correctly noticed the missing part. I'm really sorry, I missed them. :oops:

One more question:
Do I need to convert the file to Unix line endings before I commit?

I use Windows XP, so the checked out source has Dos/Windows line endings.

Regards,

Biplab

Edit 1: I found that you've already applied the 2nd part. Thanks.

[MortenMacFly]

Do I need to convert the file to Unix line endings before I commit?

Hmmm... to be honest: I don't know. Actually SVN should handle this for you. When you checkout files with Windows linefeeds but the Repository is Unix based SVN will convert the files automatically on submission. I am using SmartSVN - and there it works that way... What I don't know exactly is whether this is an option of SmartSVN or the underlying base SVN client software...!?

BTW: Concerning the patch: Nevermind, I'm at home now so I did the SVN sumbit myself (had to sync the C::B code anyway.

With regards, Morten.

[Biplab]

Hopefully SVN handles this better.

Regards,

Biplab