2011-08-03 40 views
4

私はmemcpyを使って2つの文字列を一緒に追加しようとしています。最初のmemcpyにはデータが含まれています。 2つ目は追加しません。どんな考え?memcpyと連結

if (strlen(g->db_cmd) < MAX_DB_CMDS) 
{ 
     memcpy(&g->db_cmd[strlen(g->db_cmd)],l->db.param_value.val,strlen(l->db.param_value.val)); 
     memcpy(&g->db_cmd[strlen(g->db_cmd)],l->del_const,strlen(l->del_const)); 
     g->cmd_ctr++; 
} 
+4

FYI:長い文を複数の行に分割すること、または中間結果を保持するために余分な変数を持つ複数の文に分割することも許されます。 –

+0

私はいくつかの組み込みデータベース(berkeley DBは私が思うが)はデータと長さを明示的に使用することに注意したい。データはヌルで終わる必要はなく、ヌルを含むことがあります。もしあなたのプログラムがすでにDB APIから長さを持っている場合は、strlenの使用を避けてください。 –

答えて

1

ヌルターミネータをコピーしていないため、生の文字列データのみを処理しています。そうすれば、あなたの文字列はnullで終わらず、あらゆる種類の問題を引き起こす可能性があります。また、バッファに十分なスペースがあることを確認することもしていないので、結果としてbuffer overflow vulnerabilitiesとなる可能性があります。

ヌルターミネータを確実にコピーするには、コピーするバイト数に1を加えます(コピーstrlen(l->db.param_value.val) + 1バイト)。 strlen(g->db_cmd)第二に呼び出されたときに

1

一つの可能​​性のある問題は、あなたがl->db.param_value.valから「\ 0」ターミネータをコピーしていないので、あなたの最初のmemcpy()呼び出しは、必ずしもヌル終端文字列にはなりませんということですmemcpy()に電話すると、完全に偽のものが返されている可能性があります。これが問題かどうかは、g->db_cmdバッファがあらかじめゼロに初期化されているかどうかによって異なります。

は、memcpy()で何をしようとしているのかを正確に行うために使用してください。

if (strlen(g->db_cmd) < MAX_DB_CMDS) 
    { 
     strcat(g->db_cmd, l->db.param_value.val); 
     strcat(g->db_cmd, l->del_const); 
     g->cmd_ctr++; 
    } 

誰かが読むのが楽になるという利点があります。あなたはそれほどパフォーマンスが悪いと思うかもしれませんが、明示的にstrlen()の呼び出しを行っているので、私はそうは思いません。いずれにしても、私はまずそれを正しいものにすることに集中し、次にパフォーマンスについて心配します。間違ったコードは入手できないほど最適化されていません。実際には、私の次のステップはコードのパフォーマンスを向上させることではなく、バッファオーバーランの可能性が低いようにコードを改善することです(おそらくの代わりにstrlcat()のようなものに切り替えることになります)。例えば

は、g->db_cmdはchar型の配列(およびないポインタ)であれば、結果は次のようになります。

size_t orig_len = strlen(g->db_cmd); 

size_t result = strlcat(g->db_cmd, l->db.param_value.val, sizeof(g->db_cmd)); 
result = strlcat(g->db_cmd, l->del_const, sizeof(g->db_cmd)); 
g->cmd_ctr++; 

if (result >= sizeof(g->db_cmd)) { 
    // the new stuff didn't fit, 'roll back' to what we started with 
    g->db_cmd[orig_len] = '\0'; 
    g->cmd_ctr--; 
} 

strlcat()は、お使いのプラットフォームの一部ではない場合、それはかなりネット上で見つけることができます簡単に。 MSVCを使用している場合は、代わりにと同じではないstrcat_s()関数があります(strcat_s()を呼び出す結果をどのようにチェックして処理するかを変更する必要があります)。

7
size_t len = strlen(l->db.param_value.val); 

memcpy(g->db_cmd, l->db.param_value.val, len); 
memcpy(g->db_cmd + len, l->del_const, strlen(l->del_cost)+1); 

これはあなたの次を得る:

  • 少ないstrlenへの冗長呼び出し。それらのそれぞれは文字列を横断しなければならないので、これらの呼び出しを最小限に抑えることをお勧めします。
  • 第2のmemcpyは、実際に追加する必要があり、置き換える必要はありません。したがって、最初の引数は以前の呼び出しと異なる必要があります。
  • +1は、第2のmemcpyの第3引数にあります。これはNULターミネータ用です。

あなたのifステートメントも意味をなさないと思います。おそらく、もっと普通のことは、あなたがコピーしようとしているもののために十分なスペースがあることを確認することです。g->db_cmdsizeofdb_cmdが文字の配列の場合)またはヒープ割り当ての大きさをトラッキングする(mallocdb_cmdを取得した場合)。

+1

+1、これまでのIMHOの最良の解決策。 – DarkDust