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/research/software/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 `Readtilingforvertex':
: 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@localhost 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 `Writetilingtofile':
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]

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/research/software/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_FAILURE);
}
[...]

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 `Readtilingforvertex':
: 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@localhost 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]

#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_FAILURE);
}
[...]

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]

 

[Arthur J. O'Dwyer]

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_FAILURE);
}
[...]

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
<[email protected]>, 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_FAILURE);
}
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(tempfmat, "%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_FAILURE);
}
else if (rc == 0) {
fprintf(stderr, " I need a filename! \n");
}
}
[...find the bugs in this one!...]

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]

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_FAILURE);
}
[...]

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(tempfmat, "%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_FAILURE);
}
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 ... )

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. )

 

[Stuart Anderson]

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]

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.

 

[Chris Torek]

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(tempfmat, "%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(tempfmat, "%%%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).

 

[Arthur J. O'Dwyer]

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(tempfmat, "%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(tempfmat, "%%%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(tempfmat, "%%%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(tempfmat, "%s%lu%s",
"%", (long unsigned)(sizeof 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]

This would work:
sprintf(tempfmat, "%%%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(tempfmat, "%%%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().

 

[August Derleth]

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.

I do make Makefiles, but not if I'm just making a small one-off program
that tests out an algorithm or a new subroutine package or something. So
I suppose I do type cc quite a bit, but as it's just a double-tap on one
key it isn't that hard on me.

(No, I don't touch-type the traditional way. My muscles have good
memories. )

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.

Myself, I tend to make a shell script called makeproj that takes a
Makefile template and uses sed to substitute in the name of the project
(which tends to be the name of the program and the name of the main
project source file, too). That way, I can have a master directory
containing the makeproj script, the Makefile template, all other files
I'll need again and again, and a host of conveniently-named subdirectories.

I can customize my makeproj script to fit different environments (always
putting a specific header file into the new directory, for example), and
I can modify the Makefile template just as easily.

I think we're lazy in slightly different ways.

That typing stuff is for the birds.

Agreed.

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

Don't need to.