End condition for a factorising loop

Question

Good evening,

I'm having trouble with an assignement.

Basically we're required to code a program which will calculate the prime factors of a given stdin. The data may only enter the program through its stdin, be it throuh an echo or a < file.txt. The stream of data will never be greater than 80 characters (they can be numbers or not).

The functions I use in the program are read(), strotol() and strtok(), and the "irrelevant" code flows as follows:

1. Uses malloc to allocate 80 initial bytes of memory.
2. Stores in an int, through read() the number of characters read (and I believe, the last \0).
3. reallocates memory with realloc() so as to save as much memory (I know in this case it's trivial, but oh well...).

Now comes the tricky bit:

1. Since the data has to be separated with spaces, the max amount of items to check are at most: (n/2)+1, where n is the number of characters read in the upper point nº 2.
2. Creates an array of long with max size the number obtained in point nº 1.
3. Fills numbers[0] with the result of: strtol(strtok(line, delim), &end, 10).

4. Adds 1 to the counter and enters a while loop:

   while((numbers[counter] = strtol(strtok(NULL, delim), &end, 10)) != NULL) {
       if(!*end) {
           // Check whether it's 0, 1, negative, prime or extract its factors
       }
       else {
           fprintf(stderr, "\"%s\" is not a non-negative integer", end)
       }
       counter++;
   }
   

Now, here are some inputs and their outputs:

Input: echo 1 2 3 4 5 6 7 8 9 10 | ./factors

Output:

1
2    
3
    2    2
5
    2    3
7 
    2    2    2
    3    3
    2    5
Segmentation Fault (core dumped). 

Input ./factors < integers.txt Where integers contains a COLUMN of integers.

Output:

All the integers are factorised just fine and at the end it prints a:

Segmentation Fault (core dumped). 

Input: echo abc 12 13 14 15 | ./factors

Output:

"abc" is not a non-negative integer
13
    2    7
    3    5
Segmentation Fault (core dumped). 

Input: echo -1 -2 -3 -4 -5 | ./factors

Output:

"-1" is not a non-negative integer
"-2" is not a non-negative integer
"-3" is not a non-negative integer
"-4" is not a non-negative integer
"-5" is not a non-negative integer

Input: echo abc abc abc | ./factors

Output:

"abc" is not a non-negative integer

(And does not continue checking).

Input: echo 3 4 0 6 7 | ./factors

Output:

3
    2    2

(And does not continue checking).

So as far as I can see, it fails when it encounters a 0, more than one instance of a non-integer or basically at the end of a healthy integer-based stream of data.

Any idea how I could tackle this while and why is it failing so apparently randomly?

I should let you know I'm new to C...

Thank you very much in advanced.

====================================================

EDIT1: As per request, here are the code fragments which generate numbers[], and read from stdin:

char *line;
char *end;
char *delim = " \n";
int charsread, counter; 

line = malloc(80);
charsread = read(0, line, 81);
if (charsread == 0) {
    return EX_OK;
}
realloc(line, charsread);
maxelem = (charsread / 2) + 1;
long numbers[maxelem];
numbers[0] = strtol(strtok(line, delim), &end, 10);
if (!*end) {
    zeroone(numbers[0]);
}
else {
    fprintf(stderr, "\"%s\" is not a non-negative integer\n", end);
}
counter = 1;
while [...]

Answer 1

try using gdb to debug the segfault, make it dump a core when segfaulting by setting an appropriate environment variable, or run it in gdb directly. Segfault means that you are reading/writing a part of memory you shouldn't. Randomness means that you are probably smashing the stack or something. I think "printf"'s are to blame, check their arguments. You are also not checking that numbers is smaller than array length? it might overrun it?

Answer 2

Ok, let's review a few things while we're there, and see if we can fix your problem(s).

---

This is not necessary in your program:

realloc(line, charsread);

It is correct to attempt to reduce your memory footprint, but if you allocated too much, so what? It's 80 bytes. Don't overcomplicate it you already have enough on your plate.

---

This is odd:

maxelem = (charsread / 2) + 1;
long numbers[maxelem];

This is fine and will work in your case, but you could determine the number of elements more accurately by counting the groups of numbers.

---

I'd recommend trying to do the whole extraction both in the lopp

---

Regarding the actual segmentation fault... I might be wrong as I have not the opportunity to reproduce this on my machine here, but I think the error occurs because you didn't terminate your line with a \0 character. In C, as I'm sure you already know by the look of the snippets you posted, lines are usually what we call "NULL-terminated", so by convention we terminate them with the value 0, which is the numerical value for of the NUL character, also represented as \0 (which is sometimes a bit confusing, as then people get confused between "NULL-terminated", the "NUL" char, and "NULL pointers", which are a different thing).

This then makes your program break, because eventually your program tries to read the end of the line with strtok but it doesn't know where to stop. It has no conscience of the buffer length and of where to stop in this buffer. So it keeps reading, reaching a memory address it isn't allowed to access, and thus comes up a segmentation fault.

So you'd want to simply:

/*
** If you keep your realloc, you need to allocate for the number of read
** characters from stdin, and for an extra char to terminate.
*/
line = realloc(line, charsread + 1); 
/*
** Terminate the string.
*/
line[charsread] = '\0';

Update: Ah, you were actually almost there, you had the logic but probably just missed this bit... You even wrote this yourself:

Stores in an int, through read() the number of characters read (and I believe, the last \0).

Which is partly true. If you had really gotten a line of 80 chars, your read call would have returned the line with a \0 at the end. But most of the time you have less, so your read buffer only read visible characters, and you need to null-terminate the string yourself.

---

I'd also try to rewrite your processing loop to also do the first initial call to strtok as part of it. Not always handy to write, but usually seeing a block of code nearly identical to what's inside the loop just before or just after a loop makes me think there's a better logical approach.

Answer 3

Thank you very much for your insightful response.

In the end I just rewrote the whole thing up since it didn't look as clean as I expected. Since the manual from strtok specifies a return value of NULL if no token could be extracted, this is what I ended up with:

long number;
item = strtok(line, delim);
while (item != NULL) {
       number = strtol(item, &rest, 10);
       if (*rest == 0) {
           zeroOne(number);
       }
       else {
           fprintf(stderr, "\"%s\": not a non-negative int.\n", item);
       }
       item = strtok(NULL, delim);
}   

Seems a cleaner approach and does consider the fact that the returned value from the strtok when it is NULL is returned BEFORE it tries to enter the while loop I had before. Then, this morning I came back here and read your response :)

A follow-up question regarding your hard-coded input of a \0 to the line read: Does this mean that my last strtok actually outputs the \0 token and tries to enter the loop, or does it return a NULL when it reaches the entity right after my last introduced character? As a convention, when using read() (or maybe other reading functions such as fgets() should I always try to hard-code a \0 to the line read to allow other functions to check for EOL / EOF?

If someone else should be having trouble using any of these functions (strtok and strtol) I recommend to check out these two questions posted on this very site:

Regarding strtol's second argument: Strtol second argument

Regarding strtok's output: Cannot concatenate strtok's output variable. strcat and strtok