Re: Arrays issue

In article <1148714368.704809.21630@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
<ataanis@xxxxxxxxx> wrote:
Alright , Please accept my sincere appologies, here is the complete
code of my server side, I didn't want to post the whole code thinking
that it would be too much for people to read

It is generally wise to shrink the program down (by removing
parts of it that are working) until you have a small "nugget"
of failing code, and then post that. Of course, in the process
of doing that you often find the problem yourself, but you could
consider that a bonus. :-)

[various #includes snipped]

#include <sys/types.h>
#include <sys/socket.h>
[etc]

Many of these headers are non-Standard (as in "not ANSI/ISO Standard
C", which is pretty limited) headers. Unfortunately at least a
few of them are required to demonstrate a remaining problem after
fixing the first one. Still, onward:

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

[snippage]

struct service {
int sequence;
int port;
char names[NAMESIZE];
} hostsev;

Note that hostsev.names is an array (of NAMESIZE elements) of
type "char". Thus, it can hold at most NAMESIZE "char"s, which
could be a sequence of "char"s like 'x', 'y', 'z'; if such a
sequence of "char"s ends with a '\0' character, it is a (single)
valid string that could be printed with "%s".

struct servent *h;
[snippage]
char **aliases;
//char *alias1;

"//"-comments are invalid in C89/C90, but are valid in C99. They
do have the drawback of often failing to survive line-wrapping in
postings. When I fed this code to "gcc -ansi" (same as -std=c89)
I got a pile of errors due to the invalid (for C89) //-comments.

I then fed the code to gcc without requesting C89 ("gcc -Wall -O -c"),
and I still got a pile of warnings:

x.c: In function `main':
x.c:158: warning: format argument is not a pointer (arg 2)
x.c:97: warning: unused variable `x'
x.c:95: warning: unused variable `aliases'
x.c:94: warning: unused variable `protocol1'
x.c:92: warning: unused variable `port_s'
x.c:91: warning: unused variable `message2'
x.c:90: warning: unused variable `message1'
x.c:35: warning: unused variable `port'
x.c:32: warning: unused variable `addr_size'
x.c: At top level:
x.c:21: warning: `read_from_client' declared `static' but never defined

The warning at line 158 is perhaps the most significant. Here is
some of the code leading to line 158, plus line 158 itself (without
lines that use //-comments to comment out all but whitespace). I
have adjusted indentation somewhat for posting purposes.

Note that I am going to highlight both tactical errors (mistakes
in individual steps of execution) and strategic errors (mistakes
in the overall design / plan of the program). There are quite a
few of the latter, but the former are actually bigger problems.
They are not causing the crashes you see, but require more work
to fix, eventually.

i = 0;
hostsev.names[NAMESIZE] = 0 ;

The object "hostsev.names" is, as I stated earlier, an array (of
size NAMESIZE) of "char". The valid subscripts for this array go
from 0 to NAMESIZE-1 inclusive. Setting hostsev.names[NAMESIZE]
to 0 causes undefined behavior. This could crash your program at
this point, but usually the effect of the undefined behavior is
to keep going, while maybe messing up something else later.

while (h -> s_aliases[i] != NULL) {
for (cnt = 0; h->s_aliases[i] != '\0'; ++cnt)
hostsev.names[cnt] = *h->s_aliases[i];
i++;
}

This nested pair of loops is very badly wrong, and if you ever
encountered a service with extra aliases, it would probably crash
your program even before line 158.

Here "h->s_aliases" comes from one of those non-Standard headers.
Luckily I happen to know what is in it (because I was involved in
some of the projects that created those headers, back in the 1980s).
It happens to be an object of type "pointer to pointer to char"
(or "char **"), which points to the first of one or more
valid "pointer to char"s, with the last of those "pointer to char"s
set to NULL. Any "pointer to char"s before that last one are
valid pointers that point to the first of one or more "char"s,
with the last of those "char"s in turn being a '\0'.

Pictorially, this comes out to, for instance:

h->s_aliases (unnamed block of pointers)
+-----+ +-----+
| *--|------> | *--|-> {'a', 'b', 'c', '\0' }
+-----+ +-----+
| *--|---> {'d', 'e', 'f', 'g', 'h', '\0' }
+-----+
| *--|--------> {'i', 'j', '\0' }
+-----+
| NULL|
+-----+

(if "h" happens to point to a service with three aliases "abc",
"defgh", and "ij").

The "while" loop -- which stylistically might be better written
as a "for" loop -- runs through the non-NULL aliases. If you
had written, for instance:

for (i = 0; h->s_aliases[i] != NULL; i++)
printf("%s\n", h->s_aliases[i]);

this would print "abc\n", "defgh\n", and "ij\n" respectively for
the three non-NULL pointers in the illustration above. But this
is not what appears inside the loop. Instead, you refer (in an
inner "for" loop) to *h->aliases[i]. The binding on this is such
that it means the same as *(h->aliases[i]), which is in turn the
same as h->alisaes[i][0]. What you have written can be re-expressed
thus:

for (i = 0; h->s_aliases[i] != NULL; i++)
for (cnt = 0; h->s_aliases[i] != '\0'; cnt++)
hostsev.names[cnt] = h->s_aliases[i][0];

The second line here is quite suspect: you test against '\0', but
h->s_aliases[i] is a pointer, which you (earlier, correctly) test
against NULL. In C, any integral constant zero is suitable for use
as a NULL pointer, including '\0', which is just another way to write
zero -- so this pair of loops can be re-re-expressed as:

for (i = 0; h->s_aliases[i] != NULL; i++)
for (cnt = 0; h->s_aliases[i] != NULL; cnt++)
hostsev.names[cnt] = h->s_aliases[i][0];

Now the problem is more obvious: if the outer loop runs at all,
h->s_aliases[i] is definitely not equal to NULL. The inner loop
then tests h->s_aliases[i] and runs until it is equal to NULL,
which can never occur (barring "undefined behavior" of course).
Clearly you *meant* to test h->s_aliases[i][cnt], not
h->s_aliases[i]. But that would lead to:

for (i = 0; h->s_aliases[i] != NULL; i++)
for (cnt = 0; h->s_aliases[i][cnt] != '\0'; cnt++)
hostsev.names[cnt] = h->s_aliases[i][0];

and again there is an obvious problem: the inner loop now tests
h->s_aliases[i][cnt], but copies h->s_aliases[i][0].

It has a second, slightly more subtle problem: it fails to
copy the '\0' character that terminates a C string. Presumably
the earlier assignment to the non-existent hostsev.names[NAMESIZE]
element was supposed to handle that, but this method is
hopelessly flawed: not only does it overwrite a non-existent
element, it also does not put the '\0' in the right place.

Fixing both of these is easy enough -- use strcpy() to copy
strings:

for (i = 0; h->s_aliases[i] != NULL; i++)
strcpy(hostsev.names, h->s_aliases[i]);

This still has potential bad behavior if the length of any of the
aliases is longer than fits into hostsev.names -- and of course it
illustrates the strategic error: there are, at least potentially,
a large number of aliases, each of which needs some number of
"char"s to store. The actual number needed, for any given alias,
is strlen(h->s_aliases[i])+1, where the +1 accounts for the
terminating '\0'. If there are (say) 47 aliases and each one
has an average storage requirement of 25 "char"s, you will need
25 * 47 = 1175 "char"s to hold them all. You will also need to
remember (or find) where the 47 names start -- but you have only
one "array N of char", so even if N is big enough (1175) you still
have to solve that, *if* you really want to keep all these aliases
around.

Luckily (?), h->s_aliases[i] contains only *additional* aliases
for any given service. The *primary* service name is in h->s_name.
In most cases, h->s_alises[0] is NULL, so that the outer loop does
not run at all.

Finally, if luck (whether it is good or bad) holds out, we actually
reach line 158, which reads:

printf("the content of the structure %s \n", hostsev.names[0]);

But hostsev.names[0] is a single "char". The "%s" format, in
printf, requires a valid value of type "char *", pointing to the
first of zero or more non-'\0' characters, terminated by a '\0'
character. It then prints each of the non-'\0' characters, as
if you had done:

for (i = 0; valid_ptr[i] != '\0'; i++)
putchar(valid_ptr[i]);

If you take a single "char" and coerce it into a pointer, the
resulting value is unlikely to be valid, so that "valid_ptr[i]"
crashes even when i is zero.
--
In-Real-Life: Chris Torek, Wind River Systems
Salt Lake City, UT, USA (40°39.22'N, 111°50.29'W) +1 801 277 2603
email: forget about it http://web.torek.net/torek/index.html
Reading email is like searching for food in the garbage, thanks to spammers.
.

Relevant Pages