RFID financial transaction

I have this code below which establishes connection to server (from device).
Basically assembles some buffer, sends it to server, gets response, parses and prints it. I would appreciate if someone can review it critically - see potential bugs, undefined behaviour, etc.

First some declarations:

typedef int s32;
typedef unsigned char u8;

#define SDK_KEY_MASK_ENTER 1
#define SDK_KEY_MASK_ESC 1

int sdkAccessFile(char*);
void postOperationsMenu(void);
int parseAndUpdateTerminalInfoFile(u8*,u8*);
void sdkPrintStr(u8*,int,int,int,int);
int inputAmountFloat(int*);
int ReadIDAny(u8*,int*);
int readTerminalName(u8*);
void sdkDispMsgBox(u8*,u8*, int, int);
int readTerminalKey(u8*);
int sdkMD5(u8*,u8*,int);
void DemoDisplayInfo(int,u8*,u8*);
s32 sdkCommCreateLink(void);
int sdkCommSendData(u8*, int,int);
int sdkCommRecvData(u8*,int,int,u8*);
int readTerminalInfo(u8*,u8*,u8*,u8*);
void sdkPrintStart(void);
char* serverErrorDesc(int);
void sdkPrintInit(void);
void sdkDispClearScreen(void);
int sdkReadFile(u8*,u8*,int,int*);

Code:

```
void addPoints()
{

u8 mifareCardNumber[20] = {0};
s32 idLength = 0;
//s32 keyPress = 0;
s32 amount = 0;

// Let operator enter amount first
if(false == inputAmountFloat(&amount))
return;

// Read ID
if(0 == ReadIDAny(mifareCardNumber, &idLength))
return;

//
// Sending data to server part ..
//

u8 sendBuffer[255] = {0};
s32 siRet = 0;

// Read terminal name which should be 10 bytes
u8 tName[20] = {0};
if(false == readTerminalName(tName))
{
sdkDispMsgBox("Status", "Error reading \rterminal name", 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);
return;
}

// Read authentication key
u8 key[32] = {0};
if(false == readTerminalKey(key))
{
sdkDispMsgBox("Status", "Error reading key", 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);
return;
}

//
// Assemble buffer to send to server
//
sendBuffer[0] = 1;

I see a number of things that may help you improve your code.

Break up the code into smaller functions

The addPoints() function is quite long and does a series of identifiable steps. Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.

Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 20, 200, 50, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "20" and then trying to determine if this particular 20 means the length of the card number or some other constant that happens to have the same value.

Be careful with signed versus unsigned

Your code declares pch to be u8 but then it's assigned a value from strtok which returns a char . It may be that your code is compiled to always generate unsigned char, even for constant strings, but you should make sure you know which you have.

Check for NULL

The first call to strtok could return NULL if the first character in the buffer is '\0', but the code does not seem to check for that possibility. Since we don't have the code for the various other functions, we can't tell if they are OK getting a NULL pointer, but you may wish to verify that.

Use strncpy rather than strcpy

The code currently gets up to 512 bytes from a server and parses using strtok. It then copies those parsed strings into fixed size buffers on the stack, but does not specify a maximum length. This is a recipe for creating a stack buffer overflow vulnerability. Instead, you should use the safer strncpy function which copies up to a fixed length.

Use snprintf rather than sprintf

The rationale is the same as for strncpy rather than strcpy. Additionally, you may want to limit the sizes of the strings and numbers you print by using the format specifier.

Initialize variable only when needed

The code currently calls readTerminalInfo with a number of strings that are all declared like this:

u8 terminalName[10 + 1]       = {0};

There seems to be no reason to initialize the first byte (only) to 0 if the call will fill in those values anyway. The same is true for a number of variables.

Consider consolidating the strings

If you ever need to provide a different language for the interface, it's convenient to have all of strings extracted into a list and named. For example:

// these strings should be translated
static const char STATUS_MSG[] = "Status";
static const char STATUS_READNAME_ERR = "Error reading \rterminal name"
static const char STATUS_READKEY_ERR = "Error reading key"
...
// end of translated strings

sdkDispMagBox(STATUS_MSG, STATUS_READKEY_ERR, ...);

Now all of the human translatable strings are all together and strings that should not be translated (such as ";" in this code), are separate. That way, if you ever have to produce, say, a French language version, it is more easily done. In fact, one could put each language in its own translation unit and then customization could be as simple as choosing which file to include. Note, too, that this costs literally nothing in terms of ROM or RAM or runtime. The resulting code is the same. The only difference is the organization of it.

Understand numeric conversion

Several places in the code have something like this:

sprintf(printBuffer, "Amount transferred: %.2f", (double)amount/(double)100.0);

However, neither cast is needed and the code could be written:

sprintf(printBuffer, "Amount transferred: %.2f", amount/100.0);

The reason is that the default for floating point literals is to make them double and once one of the operands is a double the other will be promoted. See this explanation of implicit conversions for more details.

Prefer a switch to long if .. else

The code currently includes a relatively long if .. else construct to parse the buffer received from the server. This, however, would be improved for readability by the use of a switch instead, and for reliability by also providing a default case that could handle the error condition (more than 5 fields).

Use the return value of sprintf

Right now there are multiple instances of code like this:

memset(printBuffer, 0, 200);
sprintf(printBuffer, "Card owner: %s", name);
sdkPrintStr(printBuffer, 1, 1, 0, 3);

First, is it really necessary to clear the buffer every time? If not, then it could simply be omitted. If so, however, it could be made slightly more efficient by using the return value of sprintf which, if the call was successful, returns the number of characters printed.

Check return codes

Some of your functions, such as parseAndUpdateTerminalInfoFile return a value. We don't know what that value is, but if it's a status code indicating success or failure, it

StackExchange Code Review Q#93073, answer score: 4