Arrays issue

[Andrew Poelstra]

Richard,
Absolutely nothing else was forgotten
thanks

First rule of C programming: Heed _everything_ that Richard
Heathfield says.

 

[Barry Schwarz]

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

No you need to tell us which printf you think generates a segmentation
fault.

Since you are using a bunch of non-standard structures and functions,
it would help to have the declarations of those also.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/select.h>
#include <string.h>
#include <netdb.h>


#define MYPORT 53491 /* the port users will be connecting to */

#define MSGSIZE 128

#define NAMESIZE 2

static int read_from_client (int fd, struct sockaddr_in *address);

/**
Accepts a string on the command line, sends that string to any
client that connects.
*/
int main (int argc, char *argv[])
{
char *message = "Waiting for another request";
int sockfd; /* datagram socket (2-way) */
struct sockaddr_in my_addr; /* my address */
int addr_size;
int enable = 1;
struct servent *h;
int port;
struct service {
int sequence;
int port;
char names[NAMESIZE];
} hostsev;



/* --- Check args and set up the message --- */
if (argc > 2) { /* usage message */
fprintf(stderr, "Usage: myserver port\n");
exit(1);
}
if (argc == 2) { /* take message from command line */
message = argv[1];
}

/* --- Create the socket --- */
if ((sockfd = socket(AF_INET, SOCK_DGRAM, 0)) == -1) {
perror("Can't create socket");
exit(1);
}

/* --- Bind the socket, making its address reusable --- */
my_addr.sin_family = AF_INET;
my_addr.sin_port = htons(MYPORT);
my_addr.sin_addr.s_addr = htonl(INADDR_ANY);
/* Note the use of INADDR_ANY to get the OS to use whatever local
interfaces are found. */

if (setsockopt(sockfd,
SOL_SOCKET, SO_REUSEADDR,
&enable, sizeof(int)) == -1) {
perror("Can't set socket option");
exit(1);
}

if (bind(sockfd,
(struct sockaddr *)&my_addr,

my_addr is a struct sockaddr_in. Here you cast its address to struct
sockaddr *. Are you ABSOLUTELY sure that my_addr and each of its
members is properly aligned to be treated as a struct sockaddr? If
not, this could invoke undefined behavior either here in the code that
generates the argument or somewhere in bind() where the argument is
used.

sizeof(my_addr)) == -1) {
perror("Could not bind");
exit(1);
}

/* --- No listen or accept code here - just wait for clients to make
requests --- */

/* --- Accept datagrams and serve our message --- */
while (1) {
struct sockaddr client_addr; /* client's address */
socklen_t addr_size = sizeof(struct sockaddr);
char client_buffer[MSGSIZE];
int readsz = 0;
char *name;
char *message1;
char *message2;
char *port_s;
char *protocol;
char *protocol1;
char **aliases;
//char *alias1;
int x = 1;
int i = 0;
int cnt;
char *SeqNumber;

/* Read from the client, get the client address filled in. Then we
can respond back to the same client. */
// memset(&client_addr, '\0', sizeof(struct sockaddr));

Is all bits zero a valid value for every possible member of
client_addr? If not, this could possibly cause undefined behavior in
recvfrom.

if ((readsz = recvfrom(sockfd, client_buffer, MSGSIZE, 0,&client_addr,
&addr_size)) > 0)
{
/* cast the client address to internet format for convenience */
struct sockaddr_in *in_addr = (struct sockaddr_in *)&client_addr;

Another potential alignment problem, this time in the opposite
direction.

client_buffer[readsz] = '\0';
/* take apart the address and see what we have, then print the
message */
fprintf(stderr, "Client: family=%d, port=%d, addr=%s,
length=%d\n",client_addr.sa_family,ntohs(in_addr->sin_port),inet_ntoa(in_addr->sin_addr),addr_size);

Do the first three arguments after the format string truly match the
types implied by the conversion specifications, int, int, and char*?

SeqNumber = strtok (client_buffer, ",");

name = strtok (NULL,",");
hostsev.sequence = atoi(SeqNumber);
printf(" there %i \n", hostsev.sequence);
while (1)
{
/* extract protocol from string sequence */
protocol = strtok(NULL,"\n");
fprintf(stderr,"Your request is the following: SeqNumber: %s, Name Of
Service: %s, Protocol: %s \n",SeqNumber,name,protocol);
//h = getservbyname(name,protocol);
if ((h = getservbyname(name,protocol)) == NULL)
{
printf("%s: unknown host %s \n",name,protocol);
exit(0);
}
else
{
printf("The port Number for your request is %d \n",ntohs(h
->s_port));
hostsev.port = h -> s_port;
printf("here %i \n" , ntohs(hostsev.port));

//port_s = atoi(itoa(ntohs(h -> s_port));
//exit(0);
i = 0;
hostsev.names[NAMESIZE] = 0 ;
while (h -> s_aliases != NULL)
{
//printf("Alias %d : \t%s\n",i, h->s_aliases);

for(cnt = 0; h->s_aliases != '\0'; ++cnt)
hostsev.names[cnt] = *h->s_aliases;
//hostsev.names += *h->s_aliases;
//printf("the content of the structure %s \n", hostsev.names);
i++;
} // loop thru aliases
break;
}
}

printf("the content of the structure %s \n", hostsev.names[0]);
/* Next, we write our message and close the socket. */
if (sendto(sockfd, &hostsev, sizeof(hostsev), 0,&client_addr,
addr_size) == -1)
{
perror("Failed to write to client");
}
else
{
fprintf(stderr, "%s\n", message);
}
}

else {
printf("Failed to read from client\n");
}
}

return 0;
}
Thank you

Remove del for email

 

[ataanis]

Barry,

The only printf that isn't working and causing the segmentation fault
is the following
printf("the content of the structure %s \n", hostsev.names[0]);

Thanks

 

[Frank Silvermann]

First rule of C programming: Heed _everything_ that Richard
Heathfield says.

As this thread bleeds on with non-Standard slop, I am quite confident
that heeding everything that Mr. Heathfield says is an impossibly-high
standard for this young man, in particular when clc's most prodigious
contributor has already pointed out exactly where to start fixing
things. I, however, disagree with Mr. Heathfield. I think the original
mistake is not spelling 'servant' correctly, although this is not a
problem in C syntax, rather a grating error in communication generally.
frank

 

[santosh]

Barry,

The only printf that isn't working and causing the segmentation fault
is the following
printf("the content of the structure %s \n", hostsev.names[0]);

Which could be because of invoking undefined behaviour, (as Barry has
pointed out), at multiple places. When you cause undefined behaviour,
anything could happen, which in this case could be printf() failing. If
even after you rectify the undefined constructs, the same error occurs,
then we can see about what to do.

 

[ataanis]

That is why I pointed to the while loop right before it e,g. :
while (h -> s_aliases != NULL)
{
//printf("Alias %d : \t%s\n",i,
h->s_aliases);

for(cnt = 0; h->s_aliases != '\0';
++cnt)
hostsev.names[cnt] = *h->s_aliases;
//hostsev.names += *h->s_aliases;
//printf("the content of the structure
%s \n", hostsev.names);
i++;
} // loop thru aliases
break;
}
printf("the content of the structure %s \n", hostsev.names[0]);
This is the printf that isn't working

 

[Keith Thompson]

Barry,

The only printf that isn't working and causing the segmentation fault
is the following
printf("the content of the structure %s \n", hostsev.names[0]);

I've already pointed you to <http://cfaj.freeshell.org/google/>.

Read it. Read it now.

 

[Clever Monkey]

As this thread bleeds on with non-Standard slop, I am quite confident
that heeding everything that Mr. Heathfield says is an impossibly-high
standard for this young man, in particular when clc's most prodigious
contributor has already pointed out exactly where to start fixing
things. I, however, disagree with Mr. Heathfield. I think the original
mistake is not spelling 'servant' correctly, although this is not a
problem in C syntax, rather a grating error in communication generally.
frank

I can't tell if you are making a very dry joke. If so, the joke's on me!

 

[Chris Torek]

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 != NULL) {
for (cnt = 0; h->s_aliases != '\0'; ++cnt)
hostsev.names[cnt] = *h->s_aliases;
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 != NULL; i++)
printf("%s\n", h->s_aliases);

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. The binding on this is such
that it means the same as *(h->aliases), which is in turn the
same as h->alisaes[0]. What you have written can be re-expressed
thus:

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

The second line here is quite suspect: you test against '\0', but
h->s_aliases 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 != NULL; i++)
for (cnt = 0; h->s_aliases != NULL; cnt++)
hostsev.names[cnt] = h->s_aliases[0];

Now the problem is more obvious: if the outer loop runs at all,
h->s_aliases is definitely not equal to NULL. The inner loop
then tests h->s_aliases 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[cnt], not
h->s_aliases. But that would lead to:

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

and again there is an obvious problem: the inner loop now tests
h->s_aliases[cnt], but copies h->s_aliases[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 != NULL; i++)
strcpy(hostsev.names, h->s_aliases);

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)+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 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 != '\0'; i++)
putchar(valid_ptr);

If you take a single "char" and coerce it into a pointer, the
resulting value is unlikely to be valid, so that "valid_ptr"
crashes even when i is zero.

 

[Chris Torek]

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

Oops, I reversed the "former" and "latter" somehow. Strategic
errors usually require more work to fix than do tactical errors.
(Of course, either one can take out the entire program.)

(Other simple typos -- there was at least one -- are left as an
exercise to the reader. )