RFID金融交易

Question

下面有一段代码，用于建立与服务器(从设备)的连接。主要是组装一些缓冲区，将其发送到服务器，获取响应，解析并打印它。如果有人能对它进行批判性的审查，我会很感激的--看看潜在的bug，未定义的行为等等。

首先是一些声明：

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*);

代码：

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;                                   // Operation code; byte 1
   memcpy(&sendBuffer[1],  tName, 10);                  // Append terminal name; next 10 bytes
   memcpy(&sendBuffer[11], &amount, 4);                 // Append amount to be added; next 4 bytes, integer
   memcpy(&sendBuffer[15], mifareCardNumber, idLength); // Append card UID; next 4 or 7 bytes

   u8 whatToHash[255] = {0};
   u8 hashOutput[16]  = {0};

   //
   // Compute signature over the packet
   //
   memcpy(whatToHash, key, 16);
   memcpy(&whatToHash[16], sendBuffer, 1 + 10 + 4 + idLength);
   if(SDK_OK != sdkMD5(hashOutput, whatToHash, 16 + 1 + 10 + 4 + idLength))
   {
      sdkDispMsgBox("Status", "Error calculating \rhash", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
      return;
   }

   // Append hash to buffer too
   memcpy(&sendBuffer[15 + idLength], hashOutput, 16);

   //
   // Actual sending of data ...
   //

   // Create Link
   DemoDisplayInfo(SDK_DISP_LINE3, "Connecting", NULL);
   siRet = sdkCommCreateLink();
   if (SDK_OK != siRet)
   {
        sdkDispMsgBox("Status", "Error create link", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
        return;
   }

   // Send the data
   siRet = sdkCommSendData(sendBuffer, 1 + 10 + 4 + idLength + 16, 0);
   if (SDK_OK != siRet)
   {
        sdkDispMsgBox("Status", "Error send data", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
        return;
   }

   // Receive date from server
   u8 revtmp[512] = {0};
   s32 len = sdkCommRecvData(revtmp, 512 /* max amount to receive */, 10000 , NULL);
   if (len <= 0)
   {
        sdkDispMsgBox("Status", "Error receive data", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
        return;
   }

   //
   // Parse server response, which comes in form "param1;param2;param3;..."
   //

   // Note: check if these lengths will be sufficient also for UTF8 string, also compare with revtmp size if it will fit
   u8 * pch           = NULL;
   u8 name[255]       = {0};
   u8 userAmount[255] = {0};
   u8 date[50]        = {0};
   u8 tranID[50]      = {0};
   u8 cashback[50]    = {0};
   int counter        = 0;

   //
   // Extract operation status from the server response first. First byte
   // indicates if it is success, error, or information update.
   //
   pch = strtok(revtmp,";"); // extract it
   int respCode = atoi(pch); // convert to integer
   if(respCode != 1)
   {

       if(respCode == 7) // 7 is information update
       {
            parseAndUpdateTerminalInfoFile(pch, tName);
            return;
       }
       else
       {
            // Otherwise error
            sdkDispMsgBox("Status", serverErrorDesc(atoi(pch)), 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
            return;
       }
    }

   // Now, extract parameters from server response
   // Note: If the response format is different we may get unexpected result.
   // Note: Pay attention that these variables have fixed length here, to avoid oveflow

   while (pch != NULL)
   {
    if(counter == 1)
    {
        // Get card owner name
        strcpy(name, pch);
    }else if(counter == 2)
    {

        strcpy(userAmount, pch);
    }else if(counter == 3)
    {
        // Get transaction ID
        strcpy(tranID, pch);
    }
    else if (counter == 4)
    {
        // Get date
        strcpy(date, pch);
    }else if (counter == 5)
    {
        // Get cashback value
        strcpy(cashback, pch);
    }
        pch = strtok (NULL, ";");
        counter++;
    }

    sdkDispClearScreen();

    // We parsed data, first show card owner and balance on the screen
    u8 finalResult[255] = {0};
    sprintf(finalResult, "Owner: %s \rBalance: %.2f", name,  (double)strtol(userAmount,NULL,10)/(double)100.0);
    sdkDispMsgBox("Transaction OK", finalResult, 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);

    // Read terminal information
    // Note: notice fixed size maximum lengths
    u8 terminalName[10 + 1]       = {0};
    u8 terminalAddress[100 + 1]   = {0};
    u8 companyName[50 + 1]        = {0};
    u8 hotlineNumber[10 + 1 + 10] = {0};
    if(false == readTerminalInfo(terminalName, companyName, terminalAddress, hotlineNumber))
    {
        sdkDispMsgBox("Status", "Error reading file", 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);
        return;
    }

    //
    // Printing ..
    //

    sdkPrintInit();
    u8 printBuffer[200]={0};

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"%s", companyName);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer," ");
    sdkPrintStr(printBuffer,1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"Terminal: %s", terminalName);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"Address: %s", terminalAddress);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Date: %s", date);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Transaction ID: %s", tranID);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Amount transferred: %.2f", (double)amount/(double)100.0);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Cashback: %.2f",(double)strtol(cashback,NULL,10)/(double)100.0);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Current balance: %.2f", (double)strtol(userAmount,NULL,10)/(double)100.0);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

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

    memset(printBuffer, 0, 200);
    sprintf(printBuffer," ");
    sdkPrintStr(printBuffer, 1,1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"Hotline: %s", hotlineNumber);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    sdkPrintStart();

    return;

}

Answer 1

我看到了一些可以帮助您改进代码的东西。

将代码分解为较小的函数

addPoints()函数很长，并执行一系列可识别的步骤。与其把所有东西都放在一个长函数中，如果每一个离散的步骤都是它自己的功能，它将更容易阅读和维护。

消除了“幻数”

这段代码中到处都是“魔术数字”，即未命名的常量，如20、200、50等。一般来说，最好避免这种情况，并给这些常量取有意义的名称。这样，如果需要更改任何内容，您就不必搜索所有" 20“实例的代码，然后尝试确定这个特定的20是否意味着卡号的长度或其他恰好具有相同值的常量。

小心使用已签名的和未签名的

您的代码声明pch为u8 *，但随后从strtok分配了一个值，该值返回一个char *。您的代码可能被编译成始终生成无符号字符，甚至对于常量字符串也是如此，但是您应该确保知道您拥有哪一个字符。

检查NULL

如果缓冲区中的第一个字符是strtok，那么对NULL的第一个调用可以返回'\0'，但是代码似乎没有检查这种可能性。因为我们没有其他函数的代码，所以我们无法判断它们是否可以获得一个NULL指针，但是您可能希望验证这一点。

使用strncpy而不是strcpy

该代码目前从服务器获取最多512个字节，并使用strtok进行分析。然后，它将这些解析的字符串复制到堆栈上的固定大小缓冲区中，但不指定最大长度。这是创建堆栈缓冲区溢出漏洞的配方。相反，您应该使用更安全的strncpy函数，它可以复制到固定长度。

使用snprintf而不是sprintf

其基本原理与strncpy相同，而不是strcpy。此外，您可能希望使用格式说明符限制您打印的字符串和数字的大小。

仅在需要

时才初始化变量

该代码当前使用多个字符串调用readTerminalInfo，这些字符串都声明如下：

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

如果调用将填充这些值，那么似乎没有理由将第一个字节(仅)初始化为0。许多变量也是如此。

考虑合并字符串

如果您需要为接口提供一种不同的语言，那么将所有字符串提取到一个列表中并命名是很方便的。例如：

// 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, ...);

现在，所有人类可翻译的字符串都在一起，不应该被翻译的字符串(例如本代码中的";" )是分开的。这样的话，如果你必须制作一个法语版本，那就更容易了。事实上，可以将每种语言放在自己的翻译单元中，然后定制就像选择包含哪个文件一样简单。请注意，这实际上是不需要任何的ROM或RAM或运行时。生成的代码是相同的。唯一的区别是它的组织。

理解数字转换

代码中有几个地方有如下内容：

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

但是，既不需要强制转换，也可以编写代码：

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

原因是浮点文本的默认设置是使其为double，一旦其中一个操作数是double，另一个操作数将被提升。有关更多细节，请参见此隐式转换的解释。

喜欢switch而不是long if .. else

该代码目前包括一个相对较长的if .. else结构，用于解析从服务器接收到的缓冲区。然而，这将通过使用switch来提高可读性，并通过提供能够处理错误条件(超过5个字段)的default情况来提高可靠性。

使用sprintf

的返回值

现在有多个代码实例，如下所示：

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

第一，是否真的有需要每次清除缓冲区？如果没有，则可以简单地略去。但是，如果是这样，则可以通过使用sprintf的返回值来稍微提高效率，如果调用成功，则返回打印的字符数。

检查返回码

您的一些函数(如parseAndUpdateTerminalInfoFile )返回一个值。我们不知道这个值是什么，但是如果它是表示成功或失败的状态代码，那么可能应该检查和处理它。现在，代码简单地抛出了这个值。

修复您的代码格式化

支撑的压痕和放置是不一致的。修复它将提高可读性，并使代码更易于维护。

Answer 2

删除硬编码长度和幻数

目前，您有大量硬编码长度和神奇数字的代码块，如下所示：

   //
   // Assemble buffer to send to server
   //
   sendBuffer[0] = 1;                                   // Operation code; byte 1
   memcpy(&sendBuffer[1],  tName, 10);                  // Append terminal name; next 10 bytes
   memcpy(&sendBuffer[11], &amount, 4);                 // Append amount to be added; next 4 bytes, integer
   memcpy(&sendBuffer[15], mifareCardNumber, idLength); // Append card UID; next 4 or 7 bytes

   u8 whatToHash[255] = {0};
   u8 hashOutput[16]  = {0};

   //
   // Compute signature over the packet
   //
   memcpy(whatToHash, key, 16);
   memcpy(&whatToHash[16], sendBuffer, 1 + 10 + 4 + idLength);
   if(SDK_OK != sdkMD5(hashOutput, whatToHash, 16 + 1 + 10 + 4 + idLength))
   {
      sdkDispMsgBox("Status", "Error calculating \rhash", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
      return;
   }

   // Append hash to buffer too
   memcpy(&sendBuffer[15 + idLength], hashOutput, 16);

   // Send the data
   siRet = sdkCommSendData(sendBuffer, 1 + 10 + 4 + idLength + 16, 0);

当需要在事务中添加一个新字段时(在将来)，您将很难在所有硬编码的地方纠正1 + 10 + 4 + idLength + 16 + ...。相反，您应该使用一个变量来跟踪当前的长度，这样您就不必担心自己计算它了。下面是如何重写代码：

static void appendBuffer(u8 *buffer, s32 *pPos, const u8 *src, s32 length)
{
   memcpy(&buffer[*pPos], src, length);
   *pPos += length;
}

void addPoints()
{
   s32 sendLen = 0;
   // ...

   //
   // Assemble buffer to send to server
   //
   u8 operationCode = OPERATION_CODE;
   appendBuffer(sendBuffer, &sendLen, &operationCode, 1);          // Operation code; byte 1
   appendBuffer(sendBuffer, &sendLen, tName, TNAME_LEN);           // Append terminal name; next 10 bytes
   appendBuffer(sendBuffer, &sendLen, (u8 *) &amount, 4);          // Append amount to be added; next 4 bytes, integer
   appendBuffer(sendBuffer, &sendLen, mifareCardNumber, idLength); // Append card UID; next 4 or 7 bytes

   u8 whatToHash[MAX_HASH_LEN] = {0};
   u8 hashOutput[HASH_OUT_LEN] = {0};
   s32 hashLen                 = 0;

   //
   // Compute signature over the packet
   //
   appendBuffer(whatToHash, &hashLen, key, KEY_LEN);
   appendBuffer(whatToHash, &hashLen, sendBuffer, sendLen);
   if(SDK_OK != sdkMD5(hashOutput, whatToHash, hashLen))
   {
      sdkDispMsgBox("Status", "Error calculating \rhash", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
      return;
   }

   // Append hash to buffer too
   appendBuffer(sendBuffer, &sendLen, hashOutput, HASH_OUT_LEN);

   // Send the data
   siRet = sdkCommSendData(sendBuffer, sendLen, 0);

   // ...
}

Answer 3

除了其他评论之外，我想指出strtok可能存在的一个问题：

如果返回的消息中字段为空，strtok将跳过该字段，因为它将任何;字符链视为单个分隔符。因此，代码将转移所有字段值。

strtok是一个非常糟糕的解析函数，它只应该用于空间分隔的值，比如scanf，它还有很多问题。这两种方法都应该避免，最好采用手工编码的方法。