In the time between my starting to write this and now, Eric Sosman has
already provided a response which covered most of what I wrote. But in
true "Me too" fashion, I shall post it anyway.
I use this function that I wrote for inputting strings. It's meant to
return a pointer to mallocated memory holding one input string, or 0 on
error. (Personally, I prefer to use 0 to NULL when returning null
pointers.)
Me too.
It looks pretty watertight to me, but my version of lint
complains about use of deallocated pointers, etc.
In my experience, lint gets confused if you do anything much more
complicated than a simple
x = malloc(...)
use_x();
free(x);
Is this code
completely safe on all input, or have I missed something?
You've missed something. See my comment after your call to fgets.
/* Header files included in the program containing malloc_getstr */
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <limits.h>
/* This function gets a string of any length from stdin,
Why not pass the FILE* in as a parameter? That would allow you to use
it in a more general case and you could still provide a wrapper to make
using it for console input convenient.
mallocating an appropriate amount of memory. It returns
0 on string length overflow or out-of-memory. Note that
slightly more space is allocated than neccesary (from
1 to 10 characters). */
char* malloc_getstr(void)
{
size_t s=0;
char* c;
char* t;
Use variable names with more than a single character! Maybe
char* line;
char* alloc;
size_t len;
You checked the return value of malloc. Good!
{ /* out of memory */
return 0;
}
for(;
{
fgets(c+s,10,stdin);
But you forgot to check the return value of fgets! If fgets fails
before any characters are read (say, stdin is already at EOF), then the
string doesn't get null-terminated. And the next thing you do is call
strchr, which assumes a null-terminated string.
If fgets succeeds the first time, you'll be fine even if it fails
thereafter because you don't overwrite the '\0' from the previous
iteration. But you still have that possibility of failure.
My safe line input function uses getc to read one character at a time
instead of mucking with fgets and its line terminators. I find it
easier, and though I haven't tested, I imagine that not having to scan
the characters again (which your strchr does below) makes up for any
lost efficiency. Heck, since getc's a macro, it might even be more
efficient. I've never had occasion to care, though.
if(strchr(c+s,'\n')) break; /* \n lets us know if the input has
ended. Check only the most recent
input. */
What if input ends (hard EOF) before a \n is read? Since you don't
check the return value of fgets, right now you have an infinite loop.
If you do check the return value of fgets but leave the bug here,
you'll incorrectly report an error condition to the caller (and forget
all the data along the way -- more about that later).
Why all these magic numbers? Have a #define EXPAND_SIZE 10, and then
that 9 here is really just (EXPAND_SIZE - 1). Also, I have found that
it's better to expand by a factor (eg, like start at size 10, and
double when you run out of space).
{ /* new string length too long to pass as realloc's argument */
free(c);
Hmm... so now you've removed a whole bunch of characters from the input
stream and failed to tell the caller any information about them. You
didn't even say how many you removed! I think that's a bad idea.
return 0; /* error return value */
I think you're trying to multiplex too much information into your
single output interface (the return value). Right now, it's doing
double-duty as the pointer to the dynamically-allocated string and an
error indicator. This is too restrictive. One alternative is:
int malloc_getstr(char** line)
Sometimes changing to a parameter makes things inconvenient for the
caller, since they can't just do something like
puts(malloc_getstr());
anymore, but in this case that would have been a memory leak anyway, so
you don't particularly want to encourage that. So the tradeoff here is
really going from
char* line;
if(line = malloc_getstr()) {
puts(line);
free(line);
}
to
char* line;
if(0 == malloc_getstr(&line)) {
puts(line);
free(line);
}
You also might want to consider allowing the user to pass a pointer to
size_t as a parmeter which, if not NULL, the object it points to gets
the number of characters read. After all, in the function you know this
already, and it's inconvenient to the tune of O(n) for the caller to
determine.
}
s+=9;
t=realloc(c,s+10);
Just a style issue: I would prefer to start c as NULL (or 0, if you
prefer) and then use realloc instead of malloc as my allocation
function, plus arrange my code in such a way that it appears only once
(albeit in a loop).
if(!t)
{ /* out of memory */
free(c);
Here you go again, forgetting things that might be useful to the
caller. If your user goes through the trouble to type a bazillion
characters as input, do you think he'll appreciate you just forgetting
it all because of a system limitation?
return 0; /* error return value */
}
c=t;
}
*strchr(c+s,'\n')=0; /* replace the \n with a \0 */
This call is suspect. Right now, the only exit point from the loop
(without returning) is when you find a '\n', so it's safe -- but that
might (should) change.
CBFalconer has posted code to his "ggets" function (search the
archives), which is an excellent implementation of the kind of thing
you're trying to do. I have my own which behaves a little differently,
but his is already out there and probably much more peer-reviewed than
mine. Even if you stick with a home-grown one, I suggest you study his.
Josh
