Problems Updating fopen() to fopen_s() in C++

Are you checking the return value of 'fclose()'? I.e.

      if ( fclose( plltCreateFile) )
      {
         printf( "Error closing file\n" );
      }

You are converting code that is using POSIX compliant functions to Microsoft's "secure" versions. Question -- why? What is your rational? Because Microsoft said so? If you ever need to port this code it'll be a can of worms you'll have to open all over again! Besides, I wonder, are these functions really anymore secure than their Posix counterparts?

If you are prepared to go down this route I'd just bite the bullet and convert the code base to use STL streams. You'll get all the flexibility of streams with all the portability of STL (did I really say that?).

Your example rewritten is something like this...

#include <fstream>
#include <sstream>

int main() // Exception handling omitted for clarity
{
      int retval = -1;

      std::ofstream fs("myfile.txt");
      
      if(fs.is_open())
      {
            std::stringstream ssLine;

            ssLine
                  << Zebra.shareName
                  << "In\\"
                  << LineNumber
                  << ".dat";

            fs
                  << PalletCreate(ssLine.str().c_str(),palletCode,timeBuffer)
                  << std::endl;

            retval = fs.good() ? 0 : -1;
      }

      return retval;
}

>>>> You are converting code that is using POSIX compliant
>>>> functions to Microsoft's "secure" versions.

There is nothing wrong with that as long as you accept that it is not portable. The _s functions check input parameters and throw exceptions if for example you are passing a NULL pointer.

>>>> if( (fopen_s(&plltCreateFile, fqnCreate,"w")) == NULL)
The problem is that fopen_s returns errno_t (and not FILE pointer). So, the fopen_s returns 0 for success and the code in the else branch never was called. See http://msdn2.microsoft.com/en-us/library/z5hh6ee9(VS.80).aspx for more information.

Regards, Alex

I didn't say it was wrong -- I was pointing out  possibles issue of portability and offering and alternative for consideration.

>>>> I didn't say it was wrong

You said:

- Because Microsoft said so?
- it'll be a can of worms you'll have to open all over again!
- I wonder, are these functions really anymore secure than
  their Posix counterparts?

I would interpret your statements as a non-recommendation and calling that 'pointing out  possibles issue of portability' surely is a euphemism. Are you a politician?

MS made a lot of bad attempts to prevent developers from making portable code. The _s functions maybe is one of these attempts but the idea to have more secure wrappers for some old and unsafe runtime functions is good and you easily could add these function wrappers to any other system/platform if needed for portability without 'opening a can of worms'.

 

...or you could use STL streams as a viable, cross-platform, alternative, no?

Incidentally, not once did I use the word "wrong" in my post! I was just questioning the rational behind these changes to ensure they are really necessary; which, doesn't mean I am stating they are wrong.

Actually, I don't have much experience with C++ at all, so all I did was do a bit of google research on the warnings the compiler was giving me.  That's pretty much the whole rationale for it.  I just don't want to leave a legacy of crappy code behind for the next guy.  So if it isn't necessary to update it I would actually prefer to leave it as is.

Are there any significant risks in not updating it?  The DLL in question is just a small, couple 100 line program which I need to invoke Windows system calls (print, file I/O) from a PLC controller (Programmable Logic Computer, used in the automation industry).

Thanks again for the advice guys.

--Charly

Again: Are you checking the return value of 'fclose()'? I.e.

      if ( fclose( plltCreateFile) )
      {
         printf( "Error closing file\n" );
      }
back to top

No, it doesn't look like the previous developer did that.  Here is all I think he does:

if( (plltCreateFile = fopen(fqnCreate,"w")) == NULL)
{
      returnValue = (-1);

}//PLLTCREAT dat can't be created
else
{
      // New stuff
      sprintf(fqnLine,"%sIn\\line%d.dat",Zebra.shareName,LineNumber);
      fprintf(plltCreateFile,"%s\n",PalletCreate(fqnLine,palletCode,timeBuffer));

      fclose(plltCreateFile);
      plltCreateFile = NULL;

}//PLLTCREAT dat file can be created

If the code is running as a DLL, there isn't a console to print to, correct?  So what would happen if I put that code in there?  A console pops up?  It gets ignored?  Throws an exception?

Thanks for the continued help.

--Charly

Or in short : if it isn't broken, don't fix it ;)

>>>> so all I did was do a bit of google research on the
>>>> warnings the compiler was giving me.  

If the warnings only is because of fopen you can ignore it. If you have other warnings you may post them here.

The Posix functions (like any tool) are safe when used safely. If I was cynical I might think the idea is to 'scare' you into changing/writing the code to use their [Microsoft] functions just to tie you in to their compiler and hence their platform. Ho hum.

>>>> You can disable the warnings very simply by defining the following macro

Actually, you got some funny advice here.

At the one hand you should not change old code (though it is only a change from == to !=) and at the other hand you should add code to suppress warnings.

If there is any unnecessary change then code to suppress warnings is the most unnecessary one.

>>>> The Posix functions (like any tool) are safe when used safely.
Yes, and the sky is blue and the meadow is green.

evilrix, the fopen_s wasn't the problem but only the wrong interpretation of the return value. And it won't be an issue even if you will post another 100 comments with wild attacks against MS.

Ok, I don't want to get into needless debate about this, but...

(a) I wasn't advocating... I was providing information, how it is used is up to the reader.
(b) It's not a code change, it's a simple project setting, hence I stated "(set this in the project settings)".
(c) The MSDN page I provided gives details of how to do this and what is involved!

If he's happy with the warnings fine, if he's not and would like to suppress them this will do it!

>> the fopen_s wasn't the problem but only the wrong interpretation of the return value

Fine, but this doesn't change the fact the changes are unnecessary!

BTW: I am not "attacking" MS, I am just stating there is no real good reason for these functions!

Typo -- Fine, but this doesn't change the fact the changes ARE NOT unnecessary!

All I added was code to suppress the one type of warning
#pragma warning(disable : 4996):

Are there any other further ranging effects to doing this?  If anything it just makes things easier to debug, since I don't get a horde of warnings all the time.

I'm also getting this warning:
            warning LNK4075: ignoring '/EDITANDCONTINUE' due to '/INCREMENTAL:NO' specification

Can I supress this too, or is there an important problem to address here?

Thanks everyone.

--Charly

Arrrgg... it's still wrong.

Oh hell, you know what I mean -- I can't type tonight :)

Anyway, I think this question is done.

>>Are there any other further ranging effects to doing this?

Usually not. The old code will compile.

>> warning LNK4075: ignoring '/EDITANDCONTINUE' due to '/INCREMENTAL:NO'
>>specification

You can safely ignore that one. '/EDITANDCONTINUE' means that you can change the code during debugging (which in some cases can be useful) and that this is being disabled.

BTW, it would be interesting to know what is going wrong anyway - since the main difference between the two functions is extended parameter validation, chances are that you end up with the same problem again.

I would add the project macro I stated above as it will be global (and doesn't change any code). The only effect of this setting is to suppress warnings about "insecure" functions!

The other warning is because you have edit and continue enabled with incremental linking disabled. Just disable the former or enable the latter; but make sure you know what both these settings do before you decide which to change!

jkr,

There isn't anything wrong when I run the original code.  I don't get any files hanging open etc.  The problems arose when I was making changes, which I plan on not doing now.

evilrix,

I think I know what edit and continue means, but not what incremental linking is.  All I do is compile my project and copy it into my PLC test program, so I don't really need any fancy debugging features in VS.

--Charly

>>>> warning LNK4075: ignoring '/EDITANDCONTINUE' due to '/INCREMENTAL:NO' specification
>>>> Can I supress this too, or is there an important problem to address here?

It is a linker warning that only happens in debug mode. Your debugger was able to 'compile on the fly' while debugging, but the option was discarded because of project settings. Actually it is a MS bug and has *no* importance. You can safely ignore it.

>>>> BTW, it would be interesting to know what is going wrong anyway

jkr, peetm actually detected the error in the very first comment of that thread:

while fopen returns NULL if the open fails, fopen_s returns 0 if the open succeeds. So, the fclose was never reached on success.

Regards, Alex

Incremental Linking -- in detail...
http://www.airs.com/blog/archives/55

Ok, I think I have gotten all of my questions answered on this topic.  Here's how I am going to do the points:

125 to peetm for answering my initial question
75 to evilrix, itsmeandnobodyelse, and jkr for helping with all the subsequent questions.

I hope this seems fair to everyone as I greatly appreciate everyone's help.

--Charly