Originally Posted by
Structure
Code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define stringSize 512
void loopText( char *inputString );
int main( int argc, char *argv[] ) {
loopText( "3 Test" ) ;
return 0 ;
}
void loopText( char *inputString ) {
int loopAmount = 0, draw = 0, count = 0 ;
char amountString[32], textString[stringSize] ;
for (int i = 0; i < (strlen(inputString)+1); i++ ) {
if ( inputString[i] == ' ' && draw == 0 ) {
draw = 1 ; count = 0 ; continue ;
}
if ( draw == 1 ) textString[count++] = inputString[i] ;
if ( draw == 0 ) amountString[count++] = inputString[i] ;
}
loopAmount = atoi(amountString) ;
for (int i = 0; i < loopAmount; i++) {
printf( "%s", textString ) ;
}
}
This looks unnecessary complex to me though: you're basically using a state machine to determine if you're processing the amount string or if you're processing the text string, but as there can only be one valid state change, separating the loop into two loops would likely be more straightforward. More importantly, the code is vulnerable to buffer overflow since it fails to check that count does not exceed 31 for the amount string and stringSize - 1 for the text string. Actually, it also fails to null terminate the amount string, but you may have gotten lucky, e.g., perhaps the array was sufficiently zero initialised even though you did not do so explicitly, or since atoi just tries to parse as much as it can, it may have ignored possible junk after the valid portion of the numeric string.
Originally Posted by
thmm
That was my idea:
This is still vulnerable to buffer overflow though: repeat does not provide a parameter for specifying the size/max length of the destination array/output string, and thus in the loop that calls strcat, it does not (as it cannot) check that strcat will not write beyond the bounds of the destination array. A lesser issue is that it unnecessarily limits the base string to a maximum length of 31, and another lesser issue is that the use of strcat in a loop in this way can be slow since strcat has to keep finding the end of the current output string.
I might suggest an approach using dynamic memory allocation instead:
Code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
char *repeat(const char *expr)
{
char *base_str = NULL;
size_t num = (size_t)strtoul(expr, &base_str, 10);
if (expr == base_str || base_str[0] != ' ')
{
return NULL;
}
++base_str;
size_t len = strlen(base_str);
char *result = malloc(num * len + 1);
if (!result)
{
return NULL;
}
if (num == 0)
{
result[0] = '\0';
}
else
{
for (size_t i = 0; i < num; ++i)
{
strcpy(result + i * len, base_str);
}
}
return result;
}
In a way this is a more complex approach since the caller will need to deal with calling free(), but then it imposes fewer limits on the string lengths, and arguably allows the core repeat loop to remain simple while avoiding buffer overflow by computing the result string length.
To test:
Code:
size_t test_repeat(const char *expr, const char *expected)
{
char *result = repeat(expr);
size_t pass = 0;
if (expected)
{
if (!result)
{
printf("Failure: expected '%s' but received null pointer\n", expected);
}
else if (strcmp(result, expected) != 0)
{
printf("Failure: expected '%s' but received '%s'\n", expected, result);
}
else
{
pass = 1;
}
}
else if (result)
{
printf("Failure: expected null pointer but received '%s'\n", result);
}
else
{
pass = 1;
}
free(result);
return pass;
}
int main()
{
size_t passes = 0;
passes += test_repeat("3 Test", "TestTestTest");
passes += test_repeat("10 123", "123123123123123123123123123123");
passes += test_repeat("4 hey Jude ", "hey Jude hey Jude hey Jude hey Jude ");
passes += test_repeat("0 hello", "");
passes += test_repeat("123 ", "");
passes += test_repeat("5", NULL);
passes += test_repeat("-1 hello", NULL);
passes += test_repeat("hey Jude", NULL);
passes += test_repeat(" hey Jude", NULL);
passes += test_repeat("", NULL);
printf("%zu tests passed.\n", passes);
return 0;
}
