Yet another User Input Question

Discussion in 'C Programming' started by Tarique, Jan 26, 2008.

  1. Tarique

    Tarique Guest

    I have tried to restrict the no. of columns in a line oriented user
    input.Can anyone please point out
    potential flaws in this method?
    btw..
    1.I have not used dynamic memory allocation because thats not something
    i want to implement.
    2.I suppose my input line oriented .
    3.The maximum columns i need is 80
    4.There is an infinite loop in the program :)
    5.I have *not* tried to validate the data as yet.
    It looks like a good user input routine is a big headache!


    #include<stdio.h>
    #include<stdlib.h>
    #include<string.h>
    #define BUFFSIZE 80 +(2)
    #define MAXTRY 5 /*Maximum no of tries allowed on error ..use it
    later on*/

    int flushln(FILE *f) {
    int ch;
    while (('\n' != (ch = getchar( ))) && (EOF != ch))
    continue;
    return ch;
    }

    /*
    Input is line oriented..every line ends with a newline character
    Input function takes the message to be printed as a parameter
    Returns a pointer to the buffer or NULL in case of error which can be
    handled in main ( )
    */

    char *input(char *message)
    {
    static error_no = 0; /*to be improved upon*/
    static char buffer[ BUFFSIZE ];
    char *p = &buffer[ BUFFSIZE-1 ];
    int flag = -1;

    printf("%s \n", message);
    memset(buffer,-1,BUFFSIZE);
    if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    puts("Input Error");
    error_no ++; /*for later use*/
    return NULL;
    }
    else{
    if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    flag = 1; /*Input within buffer range*/

    if(flag == -1){ /*There is some garbage in the stream*/
    flushln(stdin); /*Pull off the garbage from the stream*/
    puts("Dont try it honey !");
    error_no ++; /*Later use*/
    return NULL;
    }
    }
    return buffer;
    }

    int main(void)
    {
    char *iVar;
    while( (iVar = input("Message :")) == NULL) /*Needs changes*/
    continue;
    return 0;
    }
     
    Tarique, Jan 26, 2008
    #1
    1. Advertising

  2. Tarique said:

    > I have tried to restrict the no. of columns in a line oriented user
    > input.Can anyone please point out
    > potential flaws in this method?


    It's much better than average - in fact, it's pretty good, and you've
    obviously put some thought into it. But I do have a couple of
    observations.

    <snip>

    > char *input(char *message)
    > {
    > static error_no = 0; /*to be improved upon*/
    > static char buffer[ BUFFSIZE ];
    > char *p = &buffer[ BUFFSIZE-1 ];
    > int flag = -1;
    >
    > printf("%s \n", message);
    > memset(buffer,-1,BUFFSIZE);
    > if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    > puts("Input Error");
    > error_no ++; /*for later use*/
    > return NULL;


    If fgets yields NULL, it may be because the end of input as been reached.
    (This can happen interactively if the user knows how, and will certainly
    happen at some point if the input is redirected from a file.)

    > }
    > else{
    > if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    > flag = 1; /*Input within buffer range*/
    >
    > if(flag == -1){ /*There is some garbage in the stream*/
    > flushln(stdin); /*Pull off the garbage from the stream*/


    It's probably a good idea to check whether flushln returned EOF (see above
    comment re fgets).

    > puts("Dont try it honey !");
    > error_no ++; /*Later use*/
    > return NULL;
    > }
    > }
    > return buffer;
    > }
    >
    > int main(void)
    > {
    > char *iVar;
    > while( (iVar = input("Message :")) == NULL) /*Needs changes*/
    > continue;


    I think you need to distinguish between your exception conditions. You
    clearly consider a too-long input to be erroneous. That's one. An error on
    the stream is also possible (ferror(stdin) will yield non-zero if that
    happens). And of course you could hit the end of the file (in which case
    feof(stdin) will yield non-zero). These are three very different
    conditions. The first is easily recoverable - you can ask the user to
    provide shorter input and not to be such a berk. :) The second, however,
    is more problematical, and there is no standard way to recover from such
    an error. The third probably just means that your program's work is
    complete. Clearly, these are all very different, and should be treated
    differently, but at present they are all covered by a NULL return.

    Also, remember that you're relying on a static buffer within input() -
    which is fine, it's not as if that memory is going to vanish... but it
    does mean that you can only point to one input at once. If you need to
    store information, you'll need to reserve some storage into which to copy
    it.

    --
    Richard Heathfield <http://www.cpax.org.uk>
    Email: -http://www. +rjh@
    Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
    "Usenet is a strange place" - dmr 29 July 1999
     
    Richard Heathfield, Jan 26, 2008
    #2
    1. Advertising

  3. Tarique <> writes:

    > I have tried to restrict the no. of columns in a line oriented user
    > input.Can anyone please point out
    > potential flaws in this method?
    > btw..
    > 1.I have not used dynamic memory allocation because thats not
    > something i want to implement.
    > 2.I suppose my input line oriented .
    > 3.The maximum columns i need is 80
    > 4.There is an infinite loop in the program :)
    > 5.I have *not* tried to validate the data as yet.
    > It looks like a good user input routine is a big headache!
    >
    >
    > #include<stdio.h>
    > #include<stdlib.h>
    > #include<string.h>
    > #define BUFFSIZE 80 +(2)


    safer to write (80 + 2)

    > #define MAXTRY 5 /*Maximum no of tries allowed on error ..use
    > it later on*/
    >
    > int flushln(FILE *f) {
    > int ch;
    > while (('\n' != (ch = getchar( ))) && (EOF != ch))


    You don't use the f parameter!

    > continue;
    > return ch;
    > }
    >
    > /*
    > Input is line oriented..every line ends with a newline character
    > Input function takes the message to be printed as a parameter
    > Returns a pointer to the buffer or NULL in case of error which can be
    > handled in main ( )
    > */
    >
    > char *input(char *message)
    > {
    > static error_no = 0; /*to be improved upon*/
    > static char buffer[ BUFFSIZE ];
    > char *p = &buffer[ BUFFSIZE-1 ];
    > int flag = -1;
    >
    > printf("%s \n", message);
    > memset(buffer,-1,BUFFSIZE);


    You don't need to set the whole buffer and -1 is not a good choice
    anyway. Set just the last position to '\n'. fgets is guaranteed
    never to put '\n' in that position. If it is still there the read was
    short and you are OK. If it is 0, you need to see if the previous
    char is '\n' (you got a whole line) or not (you need to remove some
    more input from the line).

    > if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    > puts("Input Error");
    > error_no ++; /*for later use*/
    > return NULL;
    > }
    > else{
    > if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    > flag = 1; /*Input within buffer range*/
    >
    > if(flag == -1){ /*There is some garbage in the stream*/


    This flag is pointless. The code below is executed is the previous
    test fails -- just negate it!

    > flushln(stdin); /*Pull off the garbage from the stream*/
    > puts("Dont try it honey !");
    > error_no ++; /*Later use*/
    > return NULL;
    > }
    > }
    > return buffer;
    > }


    --
    Ben.
     
    Ben Bacarisse, Jan 26, 2008
    #3
  4. Ben Bacarisse said:

    > Tarique <> writes:
    >
    >> I have tried to restrict the no. of columns in a line oriented user
    >> input.Can anyone please point out
    >> potential flaws in this method?
    >> btw..
    >> 1.I have not used dynamic memory allocation because thats not
    >> something i want to implement.
    >> 2.I suppose my input line oriented .
    >> 3.The maximum columns i need is 80
    >> 4.There is an infinite loop in the program :)
    >> 5.I have *not* tried to validate the data as yet.
    >> It looks like a good user input routine is a big headache!
    >>
    >>
    >> #include<stdio.h>
    >> #include<stdlib.h>
    >> #include<string.h>
    >> #define BUFFSIZE 80 +(2)

    >
    > safer to write (80 + 2)
    >
    >> #define MAXTRY 5 /*Maximum no of tries allowed on error ..use
    >> it later on*/
    >>
    >> int flushln(FILE *f) {
    >> int ch;
    >> while (('\n' != (ch = getchar( ))) && (EOF != ch))

    >
    > You don't use the f parameter!


    Excellent spot, sir! That one passed me by completely. It should of course
    be ch = getc(f) rather than ch = getchar()

    --
    Richard Heathfield <http://www.cpax.org.uk>
    Email: -http://www. +rjh@
    Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
    "Usenet is a strange place" - dmr 29 July 1999
     
    Richard Heathfield, Jan 26, 2008
    #4
  5. Tarique

    Army1987 Guest

    Tarique wrote:

    > I have tried to restrict the no. of columns in a line oriented user
    > input.Can anyone please point out
    > potential flaws in this method?

    I'm just pointing out some minor style points, as the serious issues were
    already addressed by Richard Heathfield and Ben Bacarisse.
    [...]
    > char *input(char *message)

    You could declare the parameter as const char *message, as you never write
    through it.
    > {
    > static error_no = 0; /*to be improved upon*/

    Better `static int error_no = 0;`, which still works in C99.
    [...]
    > printf("%s \n", message);

    There is no reason for a blank there, right? (Trailing blanks at the end
    of a line in a text stream aren't even guaranteed to be retained.)
    Just use `puts(message);`...
    [...]
    > if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))

    No point in decrementing p, just use p[-1].
    > flag = 1; /*Input within buffer range*/

    1 and -1 are strange values for that use, if it can only have two values
    it is more typical to use 0 and 1, so that you can just write `if (flag) `.
    > if(flag == -1){ /*There is some garbage in the stream*/

    [...]
    --
    Army1987 (Replace "NOSPAM" with "email")
     
    Army1987, Jan 26, 2008
    #5
  6. Tarique

    Tarique Guest

    Richard Heathfield wrote:

    > It's much better than average - in fact, it's pretty good, and you've
    > obviously put some thought into it. But I do have a couple of
    > observations.



    Thank You Sir!


    ...snip...

    > If fgets yields NULL, it may be because the end of input as been reached.
    > (This can happen interactively if the user knows how, and will certainly
    > happen at some point if the input is redirected from a file.)
    >


    Ok ,but i did not think about the file part.Ive never used input which
    was redirected from a file.So some work lies ahead!


    > It's probably a good idea to check whether flushln returned EOF (see above
    > comment re fgets).
    >

    Will take care of that the next time
    ...snip...

    > I think you need to distinguish between your exception conditions. You
    > clearly consider a too-long input to be erroneous. That's one. An error on
    > the stream is also possible (ferror(stdin) will yield non-zero if that
    > happens). And of course you could hit the end of the file (in which case
    > feof(stdin) will yield non-zero). These are three very different
    > conditions. The first is easily recoverable - you can ask the user to
    > provide shorter input and not to be such a berk. :) The second, however,
    > is more problematical, and there is no standard way to recover from such
    > an error. The third probably just means that your program's work is
    > complete. Clearly, these are all very different, and should be treated
    > differently, but at present they are all covered by a NULL return.
    >
    > Also, remember that you're relying on a static buffer within input() -
    > which is fine, it's not as if that memory is going to vanish... but it
    > does mean that you can only point to one input at once. If you need to
    > store information, you'll need to reserve some storage into which to copy
    > it.
    >

    Ok.Can you please point out books/sections of K & R2/internet resources
    where i can read about these errors.I've not read K & R 2 from cover to
    cover or for that matter the C Standard. :)
    Please do point out specific sections
    (I do have the c99 draft though)

    With due credits to Mr.Jack Klein
    (http://home.att.net/~jackklein/c/code/strtol.html)

    Ive come up with this code to accept valid integers
    Modifying it to accept long ,long long and unsigned is pretty simple

    It does look like there are a lot of overheads involved for an input,But
    this this the best I've come during the one and a half year of my 'C'
    experience :)!

    I have not implemented all the changes suggested as yet.
    1.The memset part is certainly a big overhead and as suggested ,i will
    change it :)
    2.Did not check the return value of flushln.Will work on it as well :)

    #include <stdio.h >
    #include <stdlib.h>
    #include <string.h>
    #include <errno.h >
    #include <limits.h>

    #define BUFFSIZE (80 +2)
    #define MAXTRY 5

    int flushln(FILE *f) {
    int ch;
    while (('\n' != (ch = getc( f ))) && (EOF != ch))
    continue;
    return ch;
    }

    char *input(char *message)
    {
    static char buffer[ BUFFSIZE ];
    char *p = &buffer[ BUFFSIZE-1 ];
    int flag = 0;

    puts(message);
    memset(buffer,-1,BUFFSIZE);
    if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    puts("Input Error");
    return NULL;
    }
    else{
    if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    flag = 1;

    if(!flag){
    flushln(stdin);
    puts("Dont try it honey !\n\n");
    return NULL;
    }
    }
    return buffer;
    }

    int getInt(void)
    {
    char *buff;
    char *end_ptr;
    long int lVal;
    int trial = MAXTRY;
    errno = 0;

    while(trial != 0)
    {
    while( (buff = input("Enter Integer :")) == NULL )
    continue;

    lVal= strtol(buff, &end_ptr, 0);

    if (ERANGE == errno){
    puts("Number Is Out of Range\n\n");
    trial--;
    errno = 0;
    memset(buff,-1,BUFFSIZE);
    }
    else if (lVal > INT_MAX){
    printf("%ld Too Large!\n\n", lVal);
    trial --;
    errno = 0;
    memset(buff,-1,BUFFSIZE);
    }
    else if (lVal < INT_MIN){
    printf("%ld Too Small!\n\n", lVal);
    trial --;
    errno = 0;
    memset(buff,-1,BUFFSIZE);
    }
    else if (end_ptr == buff){
    printf("Not a Valid Integer\n\n");
    trial --;
    errno = 0;
    memset(buff,-1,BUFFSIZE);
    }
    else
    return (int)lVal;
    }
    return '\0';
    }

    int main(void)
    {
    int temp;
    if((temp = getInt( )) != '\0')
    printf("%d\n",temp);
    else
    {
    puts("This is deliberate dear! ");
    EXIT_FAILURE;
    }
    EXIT_SUCCESS;
    }

    Are there any more errors/areas where i need to work upon.Of course i
    have mentioned I've still left a couple of them behind as of now!

    Thank You
     
    Tarique, Jan 26, 2008
    #6
  7. Tarique said:

    <snip>

    > Ok.Can you please point out books/sections of K & R2/internet resources
    > where i can read about these errors.I've not read K & R 2 from cover to
    > cover or for that matter the C Standard. :)
    > Please do point out specific sections
    > (I do have the c99 draft though)



    I would certainly recommend completing your reading of K&R2. I'm afraid I
    don't know of any good textbook that devotes a lot of time to handling
    fixed buffer length inputs.

    Hmmm.

    --
    Richard Heathfield <http://www.cpax.org.uk>
    Email: -http://www. +rjh@
    Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
    "Usenet is a strange place" - dmr 29 July 1999
     
    Richard Heathfield, Jan 26, 2008
    #7
  8. Tarique

    Paul Hsieh Guest

    On Jan 26, 5:05 am, Tarique <> wrote:
    > Richard Heathfield wrote:
    > > It's much better than average - in fact, it's pretty good, and you've
    > > obviously put some thought into it. But I do have a couple of
    > > observations.

    >
    > Thank You Sir!
    >
    > ..snip...
    >
    > > If fgets yields NULL, it may be because the end of input as been reached.
    > > (This can happen interactively if the user knows how, and will certainly
    > > happen at some point if the input is redirected from a file.)

    >
    > Ok ,but i did not think about the file part.Ive never used input which
    > was redirected from a file.So some work lies ahead!
    >
    > > It's probably a good idea to check whether flushln returned EOF (see above
    > > comment re fgets).

    >
    > Will take care of that the next time
    > ..snip...
    >
    >
    >
    > > I think you need to distinguish between your exception conditions. You
    > > clearly consider a too-long input to be erroneous. That's one. An error on
    > > the stream is also possible (ferror(stdin) will yield non-zero if that
    > > happens). And of course you could hit the end of the file (in which case
    > > feof(stdin) will yield non-zero). These are three very different
    > > conditions. The first is easily recoverable - you can ask the user to
    > > provide shorter input and not to be such a berk. :) The second, however,
    > > is more problematical, and there is no standard way to recover from such
    > > an error. The third probably just means that your program's work is
    > > complete. Clearly, these are all very different, and should be treated
    > > differently, but at present they are all covered by a NULL return.

    >
    > > Also, remember that you're relying on a static buffer within input() -
    > > which is fine, it's not as if that memory is going to vanish... but it
    > > does mean that you can only point to one input at once. If you need to
    > > store information, you'll need to reserve some storage into which to copy
    > > it.

    >
    > Ok.Can you please point out books/sections of K & R2/internet resources
    > where i can read about these errors.I've not read K & R 2 from cover to
    > cover or for that matter the C Standard. :)
    > Please do point out specific sections
    > (I do have the c99 draft though)
    >
    > With due credits to Mr.Jack Klein
    > (http://home.att.net/~jackklein/c/code/strtol.html)
    >
    > Ive come up with this code to accept valid integers
    > Modifying it to accept long ,long long and unsigned is pretty simple
    >
    > It does look like there are a lot of overheads involved for an input,But
    > this this the best I've come during the one and a half year of my 'C'
    > experience :)!
    >
    > I have not implemented all the changes suggested as yet.
    > 1.The memset part is certainly a big overhead and as suggested ,i will
    > change it :)
    > 2.Did not check the return value of flushln.Will work on it as well :)
    >
    > #include <stdio.h >
    > #include <stdlib.h>
    > #include <string.h>
    > #include <errno.h >
    > #include <limits.h>
    >
    > #define BUFFSIZE (80 +2)
    > #define MAXTRY 5
    >
    > int flushln(FILE *f) {
    > int ch;
    > while (('\n' != (ch = getc( f ))) && (EOF != ch))
    > continue;
    > return ch;
    >
    > }
    >
    > char *input(char *message)
    > {
    > static char buffer[ BUFFSIZE ];


    This function "input" is not labelled static, yet you use and return a
    static declared from within it. This is usually a sign that the code
    you are writing is not reusable.

    > char *p = &buffer[ BUFFSIZE-1 ];
    > int flag = 0;
    >
    > puts(message);
    > memset(buffer,-1,BUFFSIZE);


    So you are using -1 as a sentinel character (you should comment these
    things.)

    > if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    > puts("Input Error");
    > return NULL;
    > }
    > else{
    > if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    > flag = 1;


    Well, ok, so if the very end of your buffer has a sentinel, or if your
    buffer exactly ends with "\n\0" then set this flag. What happens if
    the use inputs character -1 (under Windows, while holding the ALT key,
    the user presses 2 5 5 on the numeric keypad) as the last character in
    the buffer then proceeds to keep inputing text? While you won't
    overflow, you will not have a '\0' anywhere in your buffer either;
    which means your going to overflow in typical C string functions in
    all the code above this point. You need to scan for the '\0' in the
    entire buffer to be sure.

    As a rule of thumb, remember that security of the C language string
    functions is always more costly that you think at first, (and more
    costly that it needed to be). In this case you have to do an extra
    scan of the buffer to make sure its terminated properly no matter
    what. This isn't the cost of security -- this is the cost of the C
    language.

    You are also learning the cost of re-inventing the wheel. For a more
    straight forward approach to user input see:

    http://www.pobox.com/~qed/userInput.html

    Using getInputFrag() as your primitive you can set policy with respect
    to overflowing input as you appear to be trying to do in your code,
    without ever having the need for doing a second pass scan.

    > if(!flag){


    You can remove the "flag = 1; if (!flag)" and just put "else" its
    place (or negate the condition in the if statement.)

    > flushln(stdin);
    > puts("Dont try it honey !\n\n");
    > return NULL;
    > }
    > }
    > return buffer;
    >
    > }
    >
    > int getInt(void)
    > {
    > char *buff;
    > char *end_ptr;
    > long int lVal;
    > int trial = MAXTRY;
    > errno = 0;
    >
    > while(trial != 0)
    > {
    > while( (buff = input("Enter Integer :")) == NULL )
    > continue;
    >
    > lVal= strtol(buff, &end_ptr, 0);
    >
    > if (ERANGE == errno){
    > puts("Number Is Out of Range\n\n");
    > trial--;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else if (lVal > INT_MAX){


    Some compilers will issue a warning telling you that this is not
    possible. (Because long int and int can be the same.)

    > printf("%ld Too Large!\n\n", lVal);
    > trial --;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);


    The above is redundant.

    Its also poor programming practice. The buffer that is returned
    happens to be declared with a size BUFFSIZE, but that's not made
    explicit by the input() function itself. Each part of your program
    should be responsible for the things it uses/touches, and where it
    needs to push that responsibility outward there has to be a way for
    those functions themselves to inform the other functions about what
    responsibilities are being passed along.

    There are two ways to solve this. One is for the input function to
    also return with the length of the input buffer (remember you can
    supply pointer parameters on input to receive more than one output in
    C.) The other is to have the buffer declared by the *calling*
    function (like fgets requires you to do) and have the length passed
    into it. In either case, then the length of the buffer will become
    known and the responsibility of the calling function -- in this case
    getInt(), not main(). Then getInt() can decide what other extra
    things it wants to do with the buffer it knows the size of.

    The fundamental problem is that this code you've written can't be
    copied out and placed into other code, unless the consumer is aware of
    the BUFFSIZE macro, the management of your buffer and the use of -1 as
    a non-valid sentinel. All of these details are irrelevant to the
    processing or parsing of integers especially in your highest level
    code (main.) main() should just be viewing the task as "I need to get
    an integer, and if I can't get one, then I should do something about
    it", and not care about the details of *HOW* the integer was
    obtained. That's what the functions getInt() and input() are supposed
    to handle.

    This is the idea of encapsulation. If you are able to separate these
    details, then you can replace any of main() or getInt() or input()
    with different code without having to maintain non-obvious
    restrictions imposed by some statically defined buffer or arbitrarily
    chosen BUFFSIZE. For example, in the future you may wish to
    dynamically allocate memory for the input string. With the current
    code, you would have to touch all three functions to make that work
    properly.

    > }
    > else if (lVal < INT_MIN){
    > printf("%ld Too Small!\n\n", lVal);
    > trial --;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else if (end_ptr == buff){
    > printf("Not a Valid Integer\n\n");
    > trial --;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else
    > return (int)lVal;
    > }
    > return '\0';


    Why not just "return 0;" ?

    > }
    >
    > int main(void)
    > {
    > int temp;
    > if((temp = getInt( )) != '\0')
    > printf("%d\n",temp);
    > else
    > {
    > puts("This is deliberate dear! ");
    > EXIT_FAILURE;
    > }
    > EXIT_SUCCESS;
    >
    > }
    >
    > Are there any more errors/areas where i need to work upon.Of course i
    > have mentioned I've still left a couple of them behind as of now!


    Your design appears to be to try to force a good integer input from
    the user. But if things don't work out, just use '\0' (== 0) instead,
    and lose the fact that an error has occurred. Personally I would
    redesign this to make a function like:

    int getInt (int * outInteger, FILE * fp);

    where outInteger is filled in with the value of the integer, the file
    stream used is passed in, and any sort of error code you care to
    declare is returned from the function. In this way that calling
    function can set policy on when it gives up on trying to receive
    input, by escalating the error message, using a default value, or
    exiting the program. The point is that you can change this policy
    without affecting the task of how you input, or how you parse numbers.

    --
    Paul Hsieh
    http://www.pobox.com/~qed/
    http://bstring.sf.net/
     
    Paul Hsieh, Jan 26, 2008
    #8
  9. Paul Hsieh <> writes:

    > On Jan 26, 5:05 am, Tarique <> wrote:

    <snip>
    >> puts(message);
    >> memset(buffer,-1,BUFFSIZE);

    >
    > So you are using -1 as a sentinel character (you should comment these
    > things.)
    >
    >> if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    >> puts("Input Error");
    >> return NULL;
    >> }
    >> else{
    >> if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    >> flag = 1;

    >
    > Well, ok, so if the very end of your buffer has a sentinel, or if your
    > buffer exactly ends with "\n\0" then set this flag. What happens if
    > the use inputs character -1 (under Windows, while holding the ALT key,
    > the user presses 2 5 5 on the numeric keypad) as the last character in
    > the buffer then proceeds to keep inputing text? While you won't
    > overflow, you will not have a '\0' anywhere in your buffer either;
    > which means your going to overflow in typical C string functions in
    > all the code above this point. You need to scan for the '\0' in the
    > entire buffer to be sure.
    >
    > As a rule of thumb, remember that security of the C language string
    > functions is always more costly that you think at first, (and more
    > costly that it needed to be). In this case you have to do an extra
    > scan of the buffer to make sure its terminated properly no matter
    > what. This isn't the cost of security -- this is the cost of the C
    > language.


    I don't understand that. If you put any non-zero value into the last
    place in the buffer you can detect all the usual input conditions.
    fgets will only ever write a zero there -- never anything else -- and
    you can tell if the line is a "normal" one because there will be a
    '\n' in the second to last place. If the last place is null and the
    previous character is not '\n' fgets saw an over long line and there
    will be no '\n' in the buffer. I don't see why you say a scan is
    needed.

    --
    Ben.
     
    Ben Bacarisse, Jan 26, 2008
    #9
  10. Tarique

    Army1987 Guest

    Paul Hsieh wrote:

    >> char *input(char *message)
    >> {
    >> static char buffer[ BUFFSIZE ];

    >
    > This function "input" is not labelled static, yet you use and return a
    > static declared from within it. This is usually a sign that the code
    > you are writing is not reusable.

    Why?

    --
    Army1987 (Replace "NOSPAM" with "email")
     
    Army1987, Jan 26, 2008
    #10
  11. Tarique

    Tarique Guest

    Paul Hsieh wrote:

    > Your design appears to be to try to force a good integer input from
    > the user. But if things don't work out, just use '\0' (== 0) instead,
    > and lose the fact that an error has occurred. Personally I would
    > redesign this to make a function like:
    >
    > int getInt (int * outInteger, FILE * fp);
    >
    > where outInteger is filled in with the value of the integer, the file
    > stream used is passed in, and any sort of error code you care to
    > declare is returned from the function. In this way that calling
    > function can set policy on when it gives up on trying to receive
    > input, by escalating the error message, using a default value, or
    > exiting the program. The point is that you can change this policy
    > without affecting the task of how you input, or how you parse numbers.


    Hmmm..Quite a few design issues out there!

    I will hopefully fix them up very soon as suggested.I wrote this code
    over a cup of coffee without getting too bothered about the larger
    implications, when I am only a student programmer(hobbyist as of
    now),without even having programming as one of my major subjects :)

    Thanks for all the ideas.
     
    Tarique, Jan 27, 2008
    #11
  12. Tarique

    henry Guest

    On Jan 26, 5:05 am, Tarique <> wrote:
    > Richard Heathfield wrote:
    > > It's much better than average - in fact, it's pretty good, and you've
    > > obviously put some thought into it. But I do have a couple of
    > > observations.

    >
    > Thank You Sir!
    >
    > ..snip...
    >
    > > If fgets yields NULL, it may be because the end of input as been reached.
    > > (This can happen interactively if the user knows how, and will certainly
    > > happen at some point if the input is redirected from a file.)

    >
    > Ok ,but i did not think about the file part.Ive never used input which
    > was redirected from a file.So some work lies ahead!
    >
    > > It's probably a good idea to check whether flushln returned EOF (see above
    > > comment re fgets).

    >
    > Will take care of that the next time
    > ..snip...
    >
    >
    >
    > > I think you need to distinguish between your exception conditions. You
    > > clearly consider a too-long input to be erroneous. That's one. An error on
    > > the stream is also possible (ferror(stdin) will yield non-zero if that
    > > happens). And of course you could hit the end of the file (in which case
    > > feof(stdin) will yield non-zero). These are three very different
    > > conditions. The first is easily recoverable - you can ask the user to
    > > provide shorter input and not to be such a berk. :) The second, however,
    > > is more problematical, and there is no standard way to recover from such
    > > an error. The third probably just means that your program's work is
    > > complete. Clearly, these are all very different, and should be treated
    > > differently, but at present they are all covered by a NULL return.

    >
    > > Also, remember that you're relying on a static buffer within input() -
    > > which is fine, it's not as if that memory is going to vanish... but it
    > > does mean that you can only point to one input at once. If you need to
    > > store information, you'll need to reserve some storage into which to copy
    > > it.

    >
    > Ok.Can you please point out books/sections of K & R2/internet resources
    > where i can read about these errors.I've not read K & R 2 from cover to
    > cover or for that matter the C Standard. :)
    > Please do point out specific sections
    > (I do have the c99 draft though)
    >
    > With due credits to Mr.Jack Klein
    > (http://home.att.net/~jackklein/c/code/strtol.html)
    >
    > Ive come up with this code to accept valid integers
    > Modifying it to accept long ,long long and unsigned is pretty simple
    >
    > It does look like there are a lot of overheads involved for an input,But
    > this this the best I've come during the one and a half year of my 'C'
    > experience :)!
    >
    > I have not implemented all the changes suggested as yet.
    > 1.The memset part is certainly a big overhead and as suggested ,i will
    > change it :)
    > 2.Did not check the return value of flushln.Will work on it as well :)
    >
    > #include <stdio.h >
    > #include <stdlib.h>
    > #include <string.h>
    > #include <errno.h >
    > #include <limits.h>
    >
    > #define BUFFSIZE (80 +2)
    > #define MAXTRY 5
    >
    > int flushln(FILE *f) {
    > int ch;
    > while (('\n' != (ch = getc( f ))) && (EOF != ch))
    > continue;
    > return ch;
    >
    > }
    >
    > char *input(char *message)
    > {
    > static char buffer[ BUFFSIZE ];
    > char *p = &buffer[ BUFFSIZE-1 ];
    > int flag = 0;
    >
    > puts(message);
    > memset(buffer,-1,BUFFSIZE);
    > if( fgets(buffer,BUFFSIZE,stdin) == NULL){
    > puts("Input Error");
    > return NULL;
    > }
    > else{
    > if((*p == -1) || ( (*p == '\0') && (*--p == '\n') ))
    > flag = 1;
    >
    > if(!flag){
    > flushln(stdin);
    > puts("Dont try it honey !\n\n");
    > return NULL;
    > }
    > }
    > return buffer;
    >
    > }
    >
    > int getInt(void)
    > {
    > char *buff;
    > char *end_ptr;
    > long int lVal;
    > int trial = MAXTRY;
    > errno = 0;
    >
    > while(trial != 0)
    > {
    > while( (buff = input("Enter Integer :")) == NULL )
    > continue;
    >
    > lVal= strtol(buff, &end_ptr, 0);
    >
    > if (ERANGE == errno){
    > puts("Number Is Out of Range\n\n");
    > trial--;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else if (lVal > INT_MAX){
    > printf("%ld Too Large!\n\n", lVal);
    > trial --;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else if (lVal < INT_MIN){
    > printf("%ld Too Small!\n\n", lVal);
    > trial --;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else if (end_ptr == buff){
    > printf("Not a Valid Integer\n\n");
    > trial --;
    > errno = 0;
    > memset(buff,-1,BUFFSIZE);
    > }
    > else
    > return (int)lVal;
    > }
    > return '\0';
    >
    > }
    >
    > int main(void)
    > {
    > int temp;
    > if((temp = getInt( )) != '\0')
    > printf("%d\n",temp);
    > else
    > {
    > puts("This is deliberate dear! ");
    > EXIT_FAILURE;
    > }
    > EXIT_SUCCESS;
    >
    > }
    >
    > Are there any more errors/areas where i need to work upon.Of course i
    > have mentioned I've still left a couple of them behind as of now!
    >
    > Thank You


    You have big problem in char *input(..). You cannot do "return
    buffer;" because buffer is array allocated in side function(stack).
    After input() returns address buffer points will be wiped out and
    value is no long there.
     
    henry, Jan 27, 2008
    #12
  13. henry said:

    > On Jan 26, 5:05 am, Tarique <> wrote:


    <snip>

    >> char *input(char *message)
    >> {
    >> static char buffer[ BUFFSIZE ];


    <snip>

    >> return buffer;
    >>
    >> }
    >>

    <snip>

    > You have big problem in char *input(..). You cannot do "return
    > buffer;" because buffer is array allocated in side function(stack).
    > After input() returns address buffer points will be wiped out and
    > value is no long there.


    Not so, because it's static, which means that it exists for the duration of
    the program.

    --
    Richard Heathfield <http://www.cpax.org.uk>
    Email: -http://www. +rjh@
    Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
    "Usenet is a strange place" - dmr 29 July 1999
     
    Richard Heathfield, Jan 27, 2008
    #13
  14. Tarique

    Tarique Guest

    henry wrote:
    > On Jan 26, 5:05 am, Tarique <> wrote:
    >> Richard Heathfield wrote:


    ....snip...

    >> char *input(char *message)
    >> {
    >> static char buffer[ BUFFSIZE ];
    >> char *p = &buffer[ BUFFSIZE-1 ];
    >> int flag = 0;
    >>

    ...snip...

    >> }
    >> return buffer;
    >>
    >> }



    > You have big problem in char *input(..). You cannot do "return
    > buffer;" because buffer is array allocated in side function(stack).
    > After input() returns address buffer points will be wiped out and
    > value is no long there.


    NO.
    The array is allocated with a static qualifier.So it remains there
    during the program execution.

    return buffer; does not return pointer to a 'local' array :)
     
    Tarique, Jan 28, 2008
    #14
  15. Tarique

    Paul Hsieh Guest

    On Jan 26, 12:45 pm, Army1987 <> wrote:
    > Paul Hsieh wrote:
    > >> char *input(char *message)
    > >> {
    > >> static char buffer[ BUFFSIZE ];

    >
    > > This function "input" is not labelled static, yet you use and
    > > return a static declared from within it. This is usually a sign
    > > that the code you are writing is not reusable.

    >
    > Why?


    If in the future if he wants to change the user input device to be
    abstracted to different input streams and he wants to run these in a
    multi-threaded environment, then he's dead isn't he? By declaring the
    function static, it essentially announces to everyone that "this is
    not intended for external consumption", and thus can be used in a
    focused way paying attention to its idiosyncrocies. In fact if you
    wanted to duplicate this into multiple modules that used in it in a re-
    entrant fashion, it would likely still make sense, since there would
    be a different instance per module.

    In general, I *never* use static buffers except in a read-only fashion
    unless its absolutely avoidable. I've just gotten too old to want to
    rewrite every line of code for every problem I solve.

    --
    Paul Hsieh
    http://www.pobox.com/~qed/
    http://bstring.sf.net/
     
    Paul Hsieh, Jan 28, 2008
    #15
  16. Tarique said:

    <snip>

    > The array is allocated with a static qualifier.So it remains there
    > during the program execution.


    Correct.

    > return buffer; does not return pointer to a 'local' array :)


    I guess this depends on what you mean by "local" (which, as far as I can
    recall, the Standard doesn't define). The array has function scope, which
    means you can't refer to it *by name* outside the function, but it has
    static storage duration, which means it continues to exist throughout the
    program. Scope and lifetime are different ideas.

    As far as it matters, however, you have the right idea: YES, Virginia, you
    *can* return a pointer to the first element in that array, and have the
    calling code dereference that pointer legally and successfully.

    Having said that, I really, really wouldn't do this with a static. Don't
    get too enamoured of statics - they're not as cool as they might seem. Me?
    I'd pass in a resizeable buffer, wrapped up in a struct together with
    other useful info.

    --
    Richard Heathfield <http://www.cpax.org.uk>
    Email: -http://www. +rjh@
    Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
    "Usenet is a strange place" - dmr 29 July 1999
     
    Richard Heathfield, Jan 28, 2008
    #16
  17. Tarique

    Army1987 Guest

    Richard Heathfield wrote:

    >> return buffer; does not return pointer to a 'local' array :)

    >
    > I guess this depends on what you mean by "local" (which, as far as I can
    > recall, the Standard doesn't define).

    It doesn't define it, but it does say that argc and argv are local to
    main, and that after longjmp, objects of automatic storage duration that
    are local to the function containing the invocation of the corresponding
    setjmp [...] are indeterminate.

    --
    Army1987 (Replace "NOSPAM" with "email")
     
    Army1987, Jan 28, 2008
    #17
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Martin
    Replies:
    3
    Views:
    398
    Martin
    Jul 24, 2003
  2. qwerty
    Replies:
    3
    Views:
    9,298
    Scott Allen
    Sep 30, 2004
  3. Serdar C.
    Replies:
    2
    Views:
    519
    Serdar C.
    May 8, 2005
  4. zdrakec

    Yet another Browser Back question

    zdrakec, Jul 7, 2005, in forum: ASP .Net
    Replies:
    7
    Views:
    599
    =?Utf-8?B?U3JlZWppdGggUmFt?=
    Jul 12, 2005
  5. Berehem
    Replies:
    4
    Views:
    567
    Lawrence Kirby
    Apr 28, 2005
Loading...

Share This Page