Experimental Physics and
Industrial Control System

"Davis, Mark via Tech-talk" <[email protected]> · Wed, 31 Jul 2019 12:38:56 +0000

Hi all (Giles in particular),

I wanted to pass on a couple of issues I ran in to while working on 
getting version 2-4-10 of the Power PMAC driver working on a machine 
running Debian Stretch.

The first issue I ran in to is rather trivial:

     This call in pmacCSAxis.cpp:
         sprintf(acc_buff, pC_->getCSAccTimeCmd(fabs(max_velocity / 
acceleration) * 1000.0).c_str());

     and this call in interactive.cpp:
         printf(buff);

The new system have vers 6.3.0 of g++, and the list of options includes 
-Werror=format-security.  This means that the use of non-constant (or 
non literal?) strings for what is normally the "format" string is 
treated as an error.  This is a reasonable safeguard, since if there was 
an error in the generation of the "format" string that resulted in a "%" 
char being in it, that could cause all kinds of trouble.

In any case, the solution is simple enough:  Simply add an actual format 
string argument: "%s".  Although in the first case, it seems like it 
would make sense to just replace sprintf() with a copy operation.

The next issue I found appears to be a difference in how the compiler 
handles this statement in pmacCommandStore::buildCommandString():
       sprintf(commandString[qtyCmdStrings], "%s %s", 
commandString[qtyCmdStrings], key.c_str());

On my older development machine running Jessie (with g++ vers 4.9.2), 
this has the intended effect, which is to add the value of key.c_str() 
to the end of the current contents of commandString[qtyCmdStrings].

On my newer machine running Stretch (with g++ vers 6.3.0), the result is 
that current contents of commandString[qtyCmdStrings] is replaced with a 
space followed by the value of key.c_str().  This call occurs in two 
places:  Once before a loop (to initialize the commandString[] element), 
so the result is what is wanted (even if mostly by accident in this 
case).  But for the call inside the loop, the result is that the 
commandString[...] value contains only a space followed by the last 
value key.c_str() had (rather than the list of commands it was intended 
to contain).

I was tempted to fix this by doing something like this:

     int curLen = strlen(commandString[qtyCmdStrings]);
     sprintf(commandString[qtyCmdStrings]+curLen, "%s %s", 
commandString[qtyCmdStrings], key.c_str());

This would PROBABLY be safe, but since I don't know exactly what the 
underlying logic of sprintf() is doing now (or perhaps some new compiler 
optimization?), it seemed safer to NOT use the same array in both the 
target and the source arguments.  So instead I changed it to copy the 
current value to a temp string each time and use that in the call:

     char curStr[1024];
     ...
     strcpy(curStr, commandString[qtyCmdStrings]);
     sprintf(commandString[qtyCmdStrings], "%s %s", curStr, key.c_str());

There are of course lots of other ways this could be fixed (and 
additional safeguards added), but this is sufficient to verify what the 
problem is with the existing code when using the newer compiler and 
libraries.

--------------------------
Mark Davis
NSCL/FRIB
Control Systems Software Engineer
[email protected]
___________________________________________________________________________________________________

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System