Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Re: Why won't this code work?

Reply
Thread Tools

Re: Why won't this code work?

 
 
John Bode
Guest
Posts: n/a
 
      08-19-2003
Eirik <(E-Mail Removed)> wrote in message news:<(E-Mail Removed)>...
> <BADLY DESIGNED C CODE>
>
> #include <stdio.h>
>
> void combo(int *, int *, int *, int);
> void div(int *, int *, float *);
>
> int main(int argc, char *argv[])
> {
> int n1;
> int n2;
> char type[100];
> int result;
> float dres;
>
> printf("Number 1.\n");
> scanf("%d", &n1);
>
> printf("Number 2.\n");
> scanf("%d", &n2);
>
> printf("Addition, subtraction, multiplication, division.\n");
> gets(type);


Replace that gets() call with this:

fgets (type, sizeof type, stdin);
if (strchr (type, '\n'))
*(strchr (type, '\n')) = 0;

Reason: if somebody's stupid enough to type more than 100 characters,
gets() will happily store the extra characters to the memory following
the array. This is called a buffer overrun, which can lead to crashes
or weird behavior. It also creates a potential security hole; many
worms such as the recent MSBlast worm exploit buffer overruns.
fgets() allows you to specify the maximum size of the array (given by
sizeof type); the function will read sizeof type - 1 characters from
standard input and save them to type.

Since fgets() stores the terminating newline to the array (if there's
room), you'll need to remove it before doing any comparisons. The
strchr() call returns a pointer to the '\n' character if it's present.
If it is, we replace it with the nul terminator.

>
> if(type=="addition")


The '==' operator cannot compare array contents. Instead, it compares
the base address of type to the base address of each string literal.
It's almost guaranteed that the string literals have different base
addresses from type, so none of the tests succeed. To compare array
contents, use strcmp() as below:

if (strcmp (type, "addition") == 0)

Repeat for each case.

> {
> combo(&n1, &n2, &result, 1);
> printf("%d + %d = %d\n", n1, n2, result);
> }
> if(type=="subtraction")
> {
> combo(&n1, &n2, &result, 2);
> printf("%d - %d = %d\n", n1, n2, result);
> }
> if(type=="multiplication")
> {
> combo(&n1, &n2, &result, 3);
> printf("%d * %d = %d\n", n1, n2, result);
> }
> if(type=="division")
> {
> div(&n1, &n2, &dres);
> printf("%d / %d = %f\n", n1, n2, dres);
> }
> return 0;
> }
>
> void combo(int *nO, int *nT, int *R, int m)
> {
> if(m == 1)
> {
> *R = *nO + *nT;
> }
> if(m==2)
> {
> *R = *nO - *nT;
> }
> if(m==3)
> {
> *R = *nO * *nT;
> }
> }
>
> void div(int *nO, int *nT, float *R)
> {
> *R = (float) *nO / *nT;
> }
>
> </BADLY DESIGNED C CODE>
>
> Explanation of silly variable names:
> *R = Result
> *nO = Number One
> *nT = Number Two
> m = Method
> n1 = Number 1
> n2 = Number 2
> dres = Division Result
>
> Stupid function names:
> combo = Combination of add, subtract and multiply
> div = Division
>
> I've tried using scanf() instead of gets, but that doesn't work either.
> It compiles with no errors or warnings whatsoever when I don't use -Wall.
> The problem is that the program stops when it is supposed to ask me
> if I want it to add, subtract, multiply or divide.
> I have tried these approaches:
>
> 1 "char *type;"&"scanf("%s", &type);"
> 2 "char type[100];"&"scanf("%s", &type);"
> 3 "char type[100];"&"scanf("%c", &type);"
> 4 "char *type;"&"gets(type);"
> 5 "char type[100];"&"gets(type);"
>
> gcc calc.c -o calc -Wall produces the following output:
>
> 1 warning: char format, pointer arg (arg 2)
> 2 warning: char format, different type arg (arg 2)
> 3 warning: char format, different type arg (arg 2)
> 4 : the 'gets' function is dangerous and should not be used.
> 5 : the 'gets' function is dangerous and should not be used.
>
> I use Mandrake GNU/Linux 9.1 with gcc 3.3 and glibc 2.3.1.
>
> I hope this question is not to stupid. Flame me if you want at
> http://www.velocityreviews.com/forums/(E-Mail Removed). Replies are also welcome at the _VERY_ same
> address!
>
> Eirik

 
Reply With Quote
 
 
 
 
Eirik
Guest
Posts: n/a
 
      08-21-2003
> Replace that gets() call with this:
>
> fgets (type, sizeof type, stdin);
> if (strchr (type, '\n'))
> *(strchr (type, '\n')) = 0;
>
> Reason: if somebody's stupid enough to type more than 100 characters,
> gets() will happily store the extra characters to the memory following
> the array. This is called a buffer overrun, which can lead to crashes
> or weird behavior. It also creates a potential security hole; many
> worms such as the recent MSBlast worm exploit buffer overruns.
> fgets() allows you to specify the maximum size of the array (given by
> sizeof type); the function will read sizeof type - 1 characters from
> standard input and save them to type.
>
> Since fgets() stores the terminating newline to the array (if there's
> room), you'll need to remove it before doing any comparisons. The
> strchr() call returns a pointer to the '\n' character if it's present.
> If it is, we replace it with the nul terminator.
>
>>


Thank you for explaining this in a way in which the human brain is able to understand. I replaced the 'if(x == 'x')' with 'if(strcmp(x, "x") == NULL)' and replaced gets with 'fgets(type, sizeof(type), stdin)', but the program stops at 'Addition**********'. How come?

Here is the code:

#include <stdio.h>

void combo(float *, float *, float *, int);
void div(float *, float *, float *);

int main(int argc, char *argv[])
{

float tal1;
float tal2;
char type[100];
float resultat;
float dres;

printf("Skriv inn tal 1.\n");
scanf("%d", &tal1);

printf("Skriv inn tal 2.\n");
scanf("%d", &tal2);

printf("1: Addisjon\n2: Subtraksjon\n3: Multiplikasjon\n4: Divisjon\n");
fgets(type, sizeof(type), stdin);
if(strchr(type, '\n'))
{
*(strchr(type, '\n')) == 0;
}

if(strcmp(type, "addition") == NULL)

{

combo(&tal1, &tal2, &resultat, 1);
printf("%d + %d = %d\n", tal1, tal2, resultat);

}

if(strcmp(type, "subtraction") == NULL)

{

combo(&tal1, &tal2, &resultat, 2);
printf("%d - %d = %d\n", tal1, tal2, resultat);

}

if(strcmp(type, "multiplication") == NULL)

{

combo(&tal1, &tal2, &resultat, 3);
printf("%d * %d = %d\n", tal1, tal2, resultat);

}

if(strcmp(type, "division") == NULL)

{

div(&tal1, &tal2, &dres);
printf("%d / %d = %f\n", tal1, tal2, dres);

}

return 0;

}

void combo(float *tE, float *tT, float *R, int m)

{

if(m == 1)
{
*R = *tE + *tT;
}
if(m==2)
{
*R = *tE - *tT;
}
if(m==3)
{
*R = *tE * *tT;
}

}

void div(float *tE, float *tT, float *R)

{

*R = (float) *tE / *tT;

}

Thank you again for answering in a nice and polite manner.
Eirik
 
Reply With Quote
 
 
 
 
Dave Thompson
Guest
Posts: n/a
 
      09-01-2003
On Thu, 21 Aug 2003 22:17:15 +0200, Eirik <(E-Mail Removed)>
wrote:
<snip>
> Thank you for explaining this in a way in which the human brain is able to understand. I replaced the 'if(x == 'x')' with 'if(strcmp(x, "x") == NULL)' and replaced gets with 'fgets(type, sizeof(type), stdin)', but the program stops at 'Addition**********'. How come?
>

Please don't use such absurdly long lines in postings.

> Here is the code:
>
> #include <stdio.h>
>
> void combo(float *, float *, float *, int);
> void div(float *, float *, float *);
>
> int main(int argc, char *argv[])


Good signature for main. One bonus point.
Although since you don't use command-line arguments, you could use
int main (void) /* or just () equivalent except for recursive calls */

> {
>
> float tal1;
> float tal2;
> char type[100];
> float resultat;
> float dres;
>

For future reference, it is usually better to use 'double' instead of
'float' unless you have so much data the space used makes a
difference. 'double' is (almost always) more precise (and wider
ranged, though that's not likely an issue for you); in the dim past,
when electronic dinosaurs ruled cyberspace, it was often slower, but
on modern systems it is usually as fast and sometimes faster because
it saves extra conversions.

> printf("Skriv inn tal 1.\n");
> scanf("%d", &tal1);
>
> printf("Skriv inn tal 2.\n");
> scanf("%d", &tal2);
>

%d is wrong for float, use %f. (Or %lf for double.)

Should check these for failure, in case user doesn't type correct
input: if( scanf( etc ) != 1 ) { do something -- for a toy program
like this, probably just print an error and exit }

> printf("1: Addisjon\n2: Subtraksjon\n3: Multiplikasjon\n4: Divisjon\n");


Aside: I think this prompt is confusing; you apparently don't want
the user to enter the number, the name with a capital letter, or both,
or indeed the name spelled as in the prompt, see below.

> fgets(type, sizeof(type), stdin);


Should check for error: if( fgets( etc ) == NULL ) { error }
or just if( ! fgets( etc ) ) { error }

FAQ 12.18 (except fgets rather than gets): the previous scanf %d
(almost certainly) left a newline in the input stream, and that is all
that is read by the fgets here. Mixing streamed scanf and
line-oriented fgets correctly is difficult; best is to either input
everything as lines, and then convert the numbers using sscanf or
strtol etc.; or since you only want to input a single word here, not
really a line, use scanf %99s. (And again, check for failure.)

> if(strchr(type, '\n'))
> {
> *(strchr(type, '\n')) == 0;
> }
>

You wanted assignment = not equality test ==.

> if(strcmp(type, "addition") == NULL)
>

Compare to 0, not NULL. It is implementation dependent whether NULL
has the value /*int*/0 or the value (void*)0, and only the former can
validly be compared to the return from strcmp. If you insist on
having a name, define your own:
#define STR_EQUAL 0 /* or */
enum { STR_EQUAL = 0 };

Note that your prompt above talked about "Addisjon" but you are
looking for input "addition" here. These are not the same; both
differences in spelling *and differences in case* matter. Pick which
you want and be consistent.

> {
>
> combo(&tal1, &tal2, &resultat, 1);
> printf("%d + %d = %d\n", tal1, tal2, resultat);
>

%d is wrong for a float (which is actually promoted to double).
Use %f or conceivably %g (and the same for double, NOT %lf).

> }
>
> if(strcmp(type, "subtraction") == NULL)
>
> {
>
> combo(&tal1, &tal2, &resultat, 2);
> printf("%d - %d = %d\n", tal1, tal2, resultat);
>
> }
>
> if(strcmp(type, "multiplication") == NULL)
>
> {
>
> combo(&tal1, &tal2, &resultat, 3);
> printf("%d * %d = %d\n", tal1, tal2, resultat);
>
> }
>

Ditto, ditto, ditto.

> if(strcmp(type, "division") == NULL)
>
> {
>
> div(&tal1, &tal2, &dres);
> printf("%d / %d = %f\n", tal1, tal2, dres);
>

There seems no point to using dres here instead of resultat.
You did get *one* of the formats correct (%f) here, though.

> }
>
> return 0;
>

Good exit status. Another bonus point.

> }
>
> void combo(float *tE, float *tT, float *R, int m)
>
> {
>
> if(m == 1)
> {
> *R = *tE + *tT;
> }
> if(m==2)
> {
> *R = *tE - *tT;
> }
> if(m==3)
> {
> *R = *tE * *tT;
> }
>
> }
>
> void div(float *tE, float *tT, float *R)
>
> {
>
> *R = (float) *tE / *tT;
>

The cast is redundant; *t is already a float.

> }
>
> Thank you again for answering in a nice and polite manner.
> Eirik


- David.Thompson1 at worldnet.att.net
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
why why why why why Mr. SweatyFinger ASP .Net 4 12-21-2006 01:15 PM
findcontrol("PlaceHolderPrice") why why why why why why why why why why why Mr. SweatyFinger ASP .Net 2 12-02-2006 03:46 PM
Cisco 2611 and Cisco 1721 : Why , why , why ????? sam@nospam.org Cisco 10 05-01-2005 08:49 AM
Why, why, why??? =?Utf-8?B?VGltOjouLg==?= ASP .Net 6 01-27-2005 03:35 PM
Why Why Why You HAVE NO IDEA MCSE 31 04-24-2004 06:40 PM



Advertisments