Мне задали этот вопрос в MS-интервью:
Найти ошибки в программе ниже, которая должна возвращать новую строку с добавленным к ней \n.
char* AddnewlinetoString(char *s)
{
char buffer[1024];
strcpy(buffer,s);
buffer[strlen(s)-1] = '\n';
return buffer;
}
Я попытался написать код сам и смог заставить его работать, сделав буферную переменную global и имеющую buffer[strlen(s)] = '\n'. Но не знал, что в нем было много других ошибок.
Я вижу несколько:
Длина строки ввода не проверена.
Что делать, если strlen(s) > 1023? Вы можете вставить строку длиной не более 1023 в буфере.
Завершение последнего char с помощью
\n
Вы переписываете последний char с помощью новой строки. Ваш \n должен идти туда, где использовался \0, и вам нужно добавить новый \0 после \n
Переменный буфер является локальным для функции, и вы возвращаете его адрес.
Память для буфера распределяется в стеке и как только функция возвращается, эта память освобождается.
Я бы сделал:
char* AddnewlinetoString(char *s) {
size_t buffLen = strlen(s) + 2; // +1 for '\n' +1 for '\0'
char *buffer = malloc(buffLen);
if(!buffer) {
fprintf(stderr,"Error allocting\n");
exit(1);
}
strcpy(buffer,s);
buffer[buffLen-2] = '\n';
buffer[buffLen-1] = 0;
return buffer;
}
Вот версия С++ без ошибок:
std::string AddnewlinetoString(std::string const& s)
{
return s + "\n";
}
И вот как я, вероятно, напишу, что в С++ 0x:
std::string AddnewlinetoString(std::string s)
{
return std::move(s += "\n");
}
Я бы также добавил, что имя метода должно придерживаться шаблона, и каждое слово должно начинаться с заглавной буквы:
char* AddNewlineToString(char *s)
{
}
пс. Спасибо Konrad, я изменил имя метода, как вы предложили
3 вещи
int len = strlen(s);
char* buffer = (char*) malloc (len + 2); // 1
strcpy(buffer,s);
buffer[len] = '\n'; // 2
buffer[len+1] = '\0'; // 3
return buffer;
Изменить: на основе комментариев
Вот исправленная версия (сообщество wiki incase, которое я пропустил)
// caller must free() returned buffer string!
char* AddnewlinetoString(char *s)
{
size_t len;
char * buffer;
if (s == NULL)
s = "";
len = strlen(s);
buffer = malloc(len+2);
if (buffer == NULL)
abort();
strcpy(buffer,s);
buffer[len] = '\n';
buffer[len+1] = 0;
return buffer;
}
Как упоминает Тони, s может быть допустимым адресом, но все же быть неправильной c-строкой, без нулевых байтов. Функция может закончиться чтением, пока не наступит segfault, или какая-то другая ужасная вещь. Хотя это все еще идиоматично C, большинство людей предпочитают подсчитывать строки (а не завершать нулевые.)
// caller must free() returned buffer string!
char* AddnewlinetoStringN(char *s, size_t len)
{
char * buffer;
if (s == NULL)
s = "";
buffer = malloc(len+1); // only add 1 byte, since there no need for the nul
if (buffer == NULL)
abort();
strncpy(buffer,s,len);
buffer[len] = '\n';
return buffer;
}
Основная проблема с этим кодом заключается в том, что он уязвим для переполнения буфера стека. Это классический пример.
В принципе, вход char * может быть сделан длиннее 1024 байта; эти дополнительные байты затем перезаписывают стек, позволяя злоумышленнику изменять указатель возврата функции, чтобы указать на их вредоносный код. Ваша программа затем невольно выполнит вредоносный код.
Microsoft, как можно ожидать, будет очень заботиться об этих видах эксплойтов, поскольку Code Red Worm лихо использовала переполнение буфера стека, чтобы атаковать сотни тысяч компьютеров, на которых запущено программное обеспечение веб-сервера IIS в 2001 году.
Нет необходимости в указателе возврата. Измените входящий указатель.
int len = strlen(s);
s[len] = '\n';
s[len + 1] = '\0';
В С++ это должно быть
std::string AddNewlineToString(const std::string& s) // pass by const reference
{
return s + '\n'; // and let optimizer optimize memory allocations
}
Для строк в стиле C это может быть
char* // we want return a mutable string? OK
AddNewlineToString(
const char* s // We don't need to change the original string, so it const.
)
{
const size_t MAX_SIZE = 1024; // if it a mutable string,
// it should have a known capacity.
size_t len = strlen(s);
if(len + sizeof("\n") // To avoid the magic number "2".
> MAX_SIZE)
return NULL; // We don't know what to do with this situation,
// the user will check the result and make a decision -
// to log, raise exception, exit(), etc.
// static // We want a thread-safe result
char* buf = new char[1024]; // so we allocate memory in the heap
// and it C-language-string but not C language :)
memcpy(buf, s, len); // Copy terminating zero, and rewrite it later? NO.
memcpy(buf + len, "\n", sizeof("\n")); // The compiler will do it in one action like
// *(int16_t*)(buf + len) = *(int16_t*)"\n";
// rather than two assignments.
return buf;
}
Можно сделать очень просто с помощью strdup:
char* AddnewlinetoString(char *s) {
char *buffer = strdup(s);
buffer[strlen(s)-1] = '\n';
return buffer;
}
Здесь простейший способ:
char* AddnewlinetoString(char *s)
{
char buffer[strlen(s)+1];
snprintf(buffer,strlen(s),"%s\n",s);
return buffer;
}