gets() fgets() with subdivide.c tilepack.c squarect.c

[Stuart Anderson]

I am looking to use Ken Stephenson's CirclePack program
(http://www.math.utk.edu/~kens/) with some tiling programs written by
Cannon, Floyd, Parry. The programs I want to use are subdivide.c
tilepack.c squarect.c . They are available from
http://www.math.vt.edu/people/floyd/...re/subdiv.html.

I tried compiling them on my Redhat 9 box, eg cc subdivide.c. When I do
so I get errors like "In function `Readtilingforv ertex':
: the `gets' function is dangerous and should not be used."

I checked the FAQ and is says gets() is BAD and I should use fgets()
instead. I found an example of a code fragment to use, eg
'This is the sort of code you've got:

{
char buf[512];
gets( buf );
/* ... more code ... */
}

which will cause a buffer overrun if more than 512 characters are placed
in the buffer and randomly overwrite other memory. This can cause a
number of hard-to-trace bugs and is just a dangerous idiom.

This is also the source of the the vulnerabilities in BIND and a huge
number of other programs--by feeding the buffer the right sequence, you
can actually get it execute code buried in the buffer---though I don't
think people will use CCP4 to break into UNIX boxes.

The code to replace it is really simple:

{
char buf[512];
fgets( buf, sizeof(buf), stdin );
/* ... more code ... */
}

The header file needed for the prototypes of both functions is still
stdio.h'

So not being a c programmer I just replaced all instances of gets() with
with the above. Now I am getting different errors.
[stuart@localhos t cpack]$ cc subdivide.c
subdivide.c: In function `Readrules':
subdivide.c:160 : parse error before "sizeof"
subdivide.c:160 : conflicting types for `stdin'
/usr/include/stdio.h:142: previous declaration of `stdin'
subdivide.c:160 : parse error before ')' token
subdivide.c:167 : warning: passing arg 3 of `fgets' makes pointer from integer without a cast
subdivide.c: In function `Readtiling':
subdivide.c:311 : parse error before "sizeof"
subdivide.c:311 : conflicting types for `stdin'
/usr/include/stdio.h:142: previous declaration of `stdin'
subdivide.c:311 : parse error before ')' token
subdivide.c:318 : warning: passing arg 3 of `fgets' makes pointer from integer without a cast
subdivide.c: In function `Readtypetiling ':
subdivide.c:461 : parse error before "sizeof"
subdivide.c:461 : conflicting types for `stdin'
/usr/include/stdio.h:142: previous declaration of `stdin'
subdivide.c:461 : parse error before ')' token
subdivide.c: In function `Readbdytiling' :
subdivide.c:596 : parse error before "sizeof"
subdivide.c:596 : conflicting types for `stdin'
/usr/include/stdio.h:142: previous declaration of `stdin'
subdivide.c:596 : parse error before ')' token
subdivide.c: In function `Subdivide':
subdivide.c:719 : parse error before "sizeof"
subdivide.c:719 : conflicting types for `stdin'
/usr/include/stdio.h:142: previous declaration of `stdin'
subdivide.c:719 : parse error before ')' token
subdivide.c: In function `Writetilingtof ile':
subdivide.c:972 : parse error before "sizeof"
subdivide.c:972 : conflicting types for `stdin'
/usr/include/stdio.h:142: previous declaration of `stdin'
subdivide.c:972 : parse error before ')' token
subdivide.c:979 : warning: passing arg 3 of `fgets' makes pointer from integer without a cast

Any ideas?

thanks ,

Stuart

[Arthur J. O'Dwyer]

On Wed, 21 Jan 2004, Stuart Anderson wrote:

I am looking to use Ken Stephenson's CirclePack program
(http://www.math.utk.edu/~kens/) with some tiling programs written by
Cannon, Floyd, Parry. The programs I want to use are subdivide.c
tilepack.c squarect.c . They are available from
http://www.math.vt.edu/people/floyd/...re/subdiv.html.
Yuck, that's old code! I really wish science-type people would
learn the modern language; it would make life a lot easier for people
like you and me. Unfortunately, a lot of algorithms today are still
being presented in the programming equivalent of Shakespearean English,
a.k.a. "K&R C." A long quote from subdivide.c:

Readrules()
{ /* loop variables */
int i,j,k;

/* standard input variable and functions */
char s[256];
extern char *gets();
extern int atoi();

FILE *fp; /* move this to the variable declaration section */
fprintf(stderr, " Read which rules file? (e.g., filename.r)\n") ;
gets(s);
if ((fp = fopen(s,"r")) == NULL)
{
fprintf(stderr, " cannot open file \n");
exit();
}
[...]
In modern standard C, this would be written something like this:

#include <stdio.h>
#include <stdlib.h>

int Readrules(void)
{
int i, j, k;
char s[256];
FILE *fp;

fprintf(stderr, " Read which rules file? (e.g., filename.r)\n") ;
fgets(s, sizeof s, stdin);
if ((fp = fopen(s, "r")) == NULL)
{
fprintf(stderr, " cannot open file \n");
exit(EXIT_FAILU RE);
}
[...]

Note the correct use of 'fgets' above. This is what your code
should look like if you want 'cc' to stop giving you errors. (By
the way, 'cc' is usually an alias for the compiler usually known as
'gcc'; that's something you should know if you read this group.
And the c.l.c-recommended way to invoke 'gcc' is

gcc -W -Wall -ansi -pedantic -O2 subdivide.c

It's longer, but it's much, much better in terms of the help and
diagnostic messages it will give you.)

I tried compiling them on my Redhat 9 box, eg cc subdivide.c. When I do
so I get errors like "In function `Readtilingforv ertex':
: the `gets' function is dangerous and should not be used."

I checked the FAQ and is says gets() is BAD and I should use fgets()
instead.
That's right. [Snipped the long FAQ quote, all of which is correct,
of course.]
So not being a c programmer I just replaced all instances of gets() with
with the above. Now I am getting different errors.
[stuart@localhos t cpack]$ cc subdivide.c
subdivide.c: In function `Readrules':
subdivide.c:160 : parse error before "sizeof"

It would have helped a whole lot if you'd actually posted at least
a few of the lines of code about which the compiler was complaining.
For example, you could have said, "Line 160 in my copy of subdivide.c
is the line I've marked below:

char s[256];
extern char *fgets(buf, sizeof buf, stdin); /** THIS LINE **/
extern int atoi();

", and then we could have told you that you're not supposed to do that.
That's my best guess as to what these error messages mean; since you
haven't told us what you tried to compile, I really have no way of
knowing whether my diagnosis is right or not.

[snipped a lot more useless error messages]

See if you can figure out how my "non-Shakespearean" sample code
using fgets() works, and then modify your code accordingly. If you
still have trouble, post the smallest complete, compilable program
that still exhibits the bug, and we'll take a look.

-Arthur

[Keith Thompson]

"Arthur J. O'Dwyer" <aj*@nospam.and rew.cmu.edu> writes:

On Wed, 21 Jan 2004, Stuart Anderson wrote: [snip] In modern standard C, this would be written something like this:

#include <stdio.h>
#include <stdlib.h>

int Readrules(void)
{
int i, j, k;
char s[256];
FILE *fp;

fprintf(stderr, " Read which rules file? (e.g., filename.r)\n") ;
fgets(s, sizeof s, stdin);
if ((fp = fopen(s, "r")) == NULL)
{
fprintf(stderr, " cannot open file \n");
exit(EXIT_FAILU RE);
}
[...]

Note the correct use of 'fgets' above. This is what your code
should look like if you want 'cc' to stop giving you errors.
Apart from being far safer than gets(), fgets() also leaves the
trailing newline in the input string (unless it runs out of room
before it sees the newline). In the above code, if the user types
"filename.r " (without the quotes), the program will try to open a file
called "filename.r \n", which is unlikely to exist.

Try something like this (minimally tested):

size_t last;
...
fgets(s, sizeof s, stdin);
last = strlen(s) - 1;
if (s[last] == '\n') {
s[last] = '\0';
}
(By the way, 'cc' is usually an alias for the compiler usually known
as 'gcc'; that's something you should know if you read this group.

That's often true on Linux systems, but it's not true in general.

[snip]

--
Keith Thompson (The_Other_Keit h) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://www.sdsc.edu/~kst>
Schroedinger does Shakespeare: "To be *and* not to be"

[Arthur J. O'Dwyer]

On Wed, 21 Jan 2004, Keith Thompson wrote:

"Arthur J. O'Dwyer" <aj*@nospam.and rew.cmu.edu> writes:

fprintf(stderr, " Read which rules file? (e.g., filename.r)\n") ;
fgets(s, sizeof s, stdin);
if ((fp = fopen(s, "r")) == NULL)
{
fprintf(stderr, " cannot open file \n");
exit(EXIT_FAILU RE);
}
[...]

Note the correct use of 'fgets' above.
The best way to find your own mistakes is to confidently assert
in a public forum that you haven't made any. ;-)

Apart from being far safer than gets(), fgets() also leaves the
trailing newline in the input string (unless it runs out of room
before it sees the newline). In the above code, if the user types
"filename.r " (without the quotes), the program will try to open a file
called "filename.r \n", which is unlikely to exist.
Indeed. Whoops.
Try something like this (minimally tested):

size_t last;
...
fgets(s, sizeof s, stdin);
last = strlen(s) - 1;
if (s[last] == '\n') {
s[last] = '\0';
}

pete posted a complete fgets() example back in the mists of
<3C***********@ mindspring.com> , but it is terribly ugly, IMNSHO. :)
Note that your version will still crash on end-of-input, and as
long as you're going to be doing an O(n) algorithm anyway, you
might as well use the slightly-more-canonical version:

char *p;

p = fgets(s, sizeof s, stdin);
if (p == NULL || feof(stdin)) {
fprintf(stderr, " end of input encountered \n");
exit(EXIT_FAILU RE);
}
p = strchr(s, '\n');
if (p != NULL)
*p = '\0';
[...]

[with a few more error-checking bits added just for kicks].
Anyway, the OP should be observing that this sort of input
validation is not for the faint of heart.
I am personally a fan of 'scanf', but even the 'scanf'
solution is a bit messy in this case:

char tempfmat[20];
int rc;

sprintf(tempfma t, "%s%d%s", "%", sizeof s, "[^\n]%*[^\n]");
for (rc = 0; rc == 0; ) {
rc = scanf(tempfmat, s);
if (rc < 0) {
fprintf(stderr, " end of input encountered \n");
exit(EXIT_FAILU RE);
}
else if (rc == 0) {
fprintf(stderr, " I need a filename! \n");
}
}
[...find the bugs in this one!...]

(By the way, 'cc' is usually an alias for the compiler usually known
as 'gcc'; that's something you should know if you read this group.

That's often true on Linux systems, but it's not true in general.

Good point. I still maintain, though, that in the context of this
group, it's "usually" true. Then again, I doubt that many people in
this group actually use the 'cc' name to invoke 'gcc', so... meh.

-Arthur

[August Derleth]

Arthur J. O'Dwyer wrote:

On Wed, 21 Jan 2004, Keith Thompson wrote:

"Arthur J. O'Dwyer" <aj*@nospam.and rew.cmu.edu> writes:

fprintf(stderr, " Read which rules file? (e.g., filename.r)\n") ;
fgets(s, sizeof s, stdin);
if ((fp = fopen(s, "r")) == NULL)
{
fprintf(stderr, " cannot open file \n");
exit(EXIT_FAILU RE);
}
[...]

Note the correct use of 'fgets' above.

The best way to find your own mistakes is to confidently assert
in a public forum that you haven't made any. ;-)
I've found precisely the same thing. Over and over.

I learn, and I think egg looks good on me.
I am personally a fan of 'scanf', but even the 'scanf'
solution is a bit messy in this case:

char tempfmat[20];
int rc;

sprintf(tempfma t, "%s%d%s", "%", sizeof s, "[^\n]%*[^\n]");
for (rc = 0; rc == 0; ) {
rc = scanf(tempfmat, s);
if (rc < 0) {
fprintf(stderr, " end of input encountered \n");
exit(EXIT_FAILU RE);
}
else if (rc == 0) {
fprintf(stderr, " I need a filename! \n");
}
}
[...find the bugs in this one!...]
gcc:23:warning: scanf() is ugly and should not be used

:)

Actually, I think it's a surprisingly elegant solution given how odd
scanf is to begin with. Especially making your own format on the fly:
Very nice. :)

I would have made sure sprintf couldn't cause a buffer overrun by
mallocing the storage for the format, something that probably would have
involved calculating how many digits long the octal representation of
the number is, but I do think your solution is perfectly OK. This is
clc, not OCD.

( ... gets()... everywhere... MUST GETS()-CLEAN ... )


(By the way, 'cc' is usually an alias for the compiler usually known
as 'gcc'; that's something you should know if you read this group.

That's often true on Linux systems, but it's not true in general.

Good point. I still maintain, though, that in the context of this
group, it's "usually" true. Then again, I doubt that many people in
this group actually use the 'cc' name to invoke 'gcc', so... meh.

I do. Can't bring myself to type the extra character. (Actually, it's
muscle memory now. :) )

--
My address is yvoregnevna gjragl-guerr gjb-gubhfnaq guerr ng lnubb qbg pbz
Note: Rot13 and convert spelled-out numbers to numerical equivalents.

[Stuart Anderson]

On Wed, 21 Jan 2004 10:12:28 +1100, Stuart Anderson wrote:

Any ideas?

thanks ,

Stuart

I think you've given me enough of an idea of what to do to get the files
to compile. I'll let you know how I go. Thanks all.

Stuart

[Randy Howard]

In article <MH************ ******@fe02.use netserver.com>, se*@sig.now says...

Arthur J. O'Dwyer wrote:

Then again, I doubt that many people in
this group actually use the 'cc' name to invoke 'gcc'

I do. Can't bring myself to type the extra character. (Actually, it's
muscle memory now. :) )

Do you really type it that often? I usually invoke it indirectly, via
CC=gcc -Wall .....[some set of project-specific arguments]
in a makefile.

Several said elsewhere recently that the first thing they do on a
new project is start writing header files. For me, the Makefile
template is the first thing to get touched.

That typing stuff is for the birds. :-)

I also have a shell alias for maek='maek'. Don't ask why.

--
Randy Howard
2reply remove FOOBAR

[Chris Torek]

In article <news:Pi******* *************** ************@un ix48.andrew.cmu .edu>
Arthur J. O'Dwyer <aj*@nospam.and rew.cmu.edu> writes:

I am personally a fan of 'scanf', but even the 'scanf'
solution is a bit messy in this case:

char tempfmat[20];
int rc;

sprintf(tempfma t, "%s%d%s", "%", sizeof s, "[^\n]%*[^\n]");

Actually, you need to generate the format %19[, not %20[, so you
need ((sizeof s) - 1) here; and %d takes an int while sizeof
produces a size_t (perhaps a 64-bit unsigned long, vs the 32-bit
int).

This would work:

sprintf(tempfma t, "%%%d[^\n]%*[^\n]", (int)sizeof s - 1);

or you could even use the fact that it is "obvious" (?) that 20
and 19 are "the same number" :-) and just encode %19[ directly.

As for myself, I am not such a fan of scanf(). The scanf-ish
functions in in the "sfio" package might be better (I have not
actually looked, but from the few examples I have heard of, it
seems to be aimed at interacting with those pesky "users" you may
have heard of).
--
In-Real-Life: Chris Torek, Wind River Systems
Salt Lake City, UT, USA (40°39.22'N, 111°50.29'W) [phone]
email: forget about it http://web.torek.net/torek/index.html
Reading email is like searching for food in the garbage, thanks to spammers.

[Arthur J. O'Dwyer]

On Wed, 21 Jan 2004, Chris Torek wrote:

Arthur J. O'Dwyer <aj*@nospam.and rew.cmu.edu> writes:

I am personally a fan of 'scanf', but even the 'scanf'
solution is a bit messy in this case:

char tempfmat[20];
int rc;

sprintf(tempfma t, "%s%d%s", "%", sizeof s, "[^\n]%*[^\n]");
Actually, you need to generate the format %19[, not %20[, so you
need ((sizeof s) - 1) here; and %d takes an int while sizeof
produces a size_t (perhaps a 64-bit unsigned long, vs the 32-bit
int).

This would work:

sprintf(tempfma t, "%%%d[^\n]%*[^\n]", (int)sizeof s - 1);

Not quite. You should now be able to see why I wrote "%s%d%s"
and a couple more string literals, rather than trying to squeeze
it all into the format string. That line *should* read

sprintf(tempfma t, "%%%d[^\n]%%*[^\n]", (int)sizeof s - 1);

to avoid undefined behavior. (Note the extra doubled % sign.)
And just to be double-triple-clc sure, we might as well use a
conversion specifier designed to handle unsigned values, in case
'sizeof s' doesn't fit in an 'int'. ;-)

sprintf(tempfma t, "%s%lu%s",
"%", (long unsigned)(sizeo f s - 1), "[^\n]%*[^\n]");
or you could even use the fact that it is "obvious" (?) that 20
and 19 are "the same number" :-) and just encode %19[ directly.

Also, looking more closely, you'll notice that 20 is not the
length of the buffer, but the length of the format string
'tempfmat'. The length of 's' is presumably not 20 -- I forget
what it was, but I'd guess 100 or 200. Which are just arbitrary
enough to make a stray %99s in a format string go undetected by
the next programmer. :)

Good catches on the off-by-one and size_t!

-Arthur

[Chris Torek]

>On Wed, 21 Jan 2004, Chris Torek wrote:

This would work:
sprintf(tempfma t, "%%%d[^\n]%*[^\n]", (int)sizeof s - 1);

In article <news:Pi******* *************** ************@un ix46.andrew.cmu .edu>
Arthur J. O'Dwyer <aj*@nospam.and rew.cmu.edu> writes: Not quite. You should now be able to see why I wrote "%s%d%s"
and a couple more string literals, rather than trying to squeeze
it all into the format string. That line *should* read

sprintf(tempfma t, "%%%d[^\n]%%*[^\n]", (int)sizeof s - 1);

to avoid undefined behavior.
Oops. Indeed. (I did realize why you had used %s directives to
get the other directives through; I just wanted to point out that
-- with care -- they can be "inlined", as it were. But it takes
even more care than I used. :-) )
And just to be double-triple-clc sure, we might as well use a
conversion specifier designed to handle unsigned values, in case
'sizeof s' doesn't fit in an 'int'. ;-)

Although this will get the right value into the format even if
the "right value" is (say) 142599 even when INT_MAX is 32767,
it is not clear this would work "under the hood", as it were.
The width specifier is listed as a "decimal integer" (not
specifically an "int"), so it *might* work -- but the Standard
only requires relatively short and output lines to be supported
in the first place. (This might explain why %* directives in
printf take an int value instead of a size_t value. Either
that, or "no one thought about it", or "everyone figured it was
too ridiculous", or "it seemed necessary to keep it as int for
backwards compatibility", or some combination of these. :-) )

All in all, I still just prefer to avoid scanf(). :-)
--
In-Real-Life: Chris Torek, Wind River Systems
Salt Lake City, UT, USA (40°39.22'N, 111°50.29'W) [phone]
email: forget about it http://web.torek.net/torek/index.html
Reading email is like searching for food in the garbage, thanks to spammers.