What's wrong with this server code ?? Cant read incoming data

[Jens]

Hello,

I have been looking for some C-code which listens on a user-defined port
for incoming data traffic. When data is received, the data is written to a
file.

I found some C-code (server) that almost does the job. It listens on a
user-defined
port and responds to incoming data by writing how many times somebody
has tried to connect to the server.

I modified the code, but the read function returns an error (-1) and
incoming data
is not written to the buffer.

Anybody out there who can tell me what I am doing wrong?

Here is the code:

/* server.c - code for example server program that uses TCP */


#ifndef unix
#define WIN32
#include <windows.h>
#include <winsock.h>
#else
#define closesocket close
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#endif

#include <stdio.h>
#include <string.h>

#define PROTOPORT 5193 /* default protocol port number */
#define QLEN 6 /* size of request queue */

int visits = 0; /* counts client connections */
int nbytes;
/*------------------------------------------------------------------------
* Program: server
*
* Purpose: allocate a socket and then repeatedly execute the following:
* (1) wait for the next connection from a client
* (2) send a short message to the client
* (3) close the connection
* (4) go back to step (1)
*
* Syntax: server [ port ]
*
* port - protocol port number to use
*
* Note: The port argument is optional. If no port is specified,
* the server uses the default given by PROTOPORT.
*
*------------------------------------------------------------------------
*/
main(argc, argv)
int argc;
char *argv[];
{
struct hostent *ptrh; /* pointer to a host table entry */
struct protoent *ptrp; /* pointer to a protocol table entry */
struct sockaddr_in sad; /* structure to hold server's address */
struct sockaddr_in cad; /* structure to hold client's address */
int sd, sd2; /* socket descriptors */
int port; /* protocol port number */
int alen; /* length of address */
char buf[1000]; /* buffer for string the server sends */

#ifdef WIN32
WSADATA wsaData;
WSAStartup(0x0101, &wsaData);
#endif
memset((char *)&sad,0,sizeof(sad)); /* clear sockaddr structure */
sad.sin_family = AF_INET; /* set family to Internet */
sad.sin_addr.s_addr = INADDR_ANY; /* set the local IP address */

/* Check command-line argument for protocol port and extract */
/* port number if one is specified. Otherwise, use the default */
/* port value given by constant PROTOPORT */

if (argc > 1) { /* if argument specified */
port = atoi(argv[1]); /* convert argument to binary */
} else {
port = PROTOPORT; /* use default port number */
}
if (port > 0) /* test for illegal value */
sad.sin_port = htons((u_short)port);
else { /* print error message and exit */
fprintf(stderr,"bad port number %s\n",argv[1]);
exit(1);
}

/* Map TCP transport protocol name to protocol number */

if ( ((int)(ptrp = getprotobyname("tcp"))) == 0) {
fprintf(stderr, "cannot map \"tcp\" to protocol number");
exit(1);
}

/* Create a socket */

sd = socket(PF_INET, SOCK_STREAM, ptrp->p_proto);
if (sd < 0) {
fprintf(stderr, "socket creation failed\n");
exit(1);
}

/* Bind a local address to the socket */

if (bind(sd, (struct sockaddr *)&sad, sizeof(sad)) < 0) {
fprintf(stderr,"bind failed\n");
exit(1);
}

/* Specify size of request queue */

if (listen(sd, QLEN) < 0) {
fprintf(stderr,"listen failed\n");
exit(1);
}

/* Main server loop - accept and handle requests */

while (1) {
alen = sizeof(cad);
if ( (sd2=accept(sd, (struct sockaddr *)&cad, &alen)) < 0) {
fprintf(stderr, "accept failed\n");
exit(1);
}
visits++;


//sprintf(buf,"This server has been contacted %d
time%s\n",visits,visits==1?".":"s.");
//send(sd2,buf,strlen(buf),0);

// read returns an error (-1)
nbytes=read(sd2,buf,strlen(buf));
printf("read %d bytes of data",nbytes);


closesocket(sd2);
}
}

 

[Ian Collins]

Hello,

I have been looking for some C-code which listens on a user-defined port
for incoming data traffic. When data is received, the data is written to a
file.

While sockets are OT here, one obvious error isn't:

char buf[1000]; /* buffer for string the server sends */

Note the array buf isn't initialised.

nbytes=read(sd2,buf,strlen(buf));

You are calling strlen on an initialised buffer.

Use a constant for the size.

 

[Ian Collins]

Hello,

I have been looking for some C-code which listens on a user-defined port
for incoming data traffic. When data is received, the data is written to a
file.

While sockets are OT here, one obvious error isn't:

char buf[1000]; /* buffer for string the server sends */

Note the array buf isn't initialised.

nbytes=read(sd2,buf,strlen(buf));

You are calling strlen on an initialised buffer.

Use a constant for the size.

Or sizeof(buf).

 

[Keith Thompson]

I have been looking for some C-code which listens on a user-defined port
for incoming data traffic. When data is received, the data is written to a
file.

[snip]

You also posted this in comp.std.c. It's off-topic in both
newsgroups. See my respones there.

 

[Flash Gordon]

Jens wrote, On 23/01/07 21:56:

Hello,

I have been looking for some C-code which listens on a user-defined port
for incoming data traffic. When data is received, the data is written to a
file.

We don't deal with networking here. There are networking groups and
groups for your implementation that would be better. However, I will
comment on the C aspects of your code.

Here is the code:

/* server.c - code for example server program that uses TCP */


#ifndef unix
#define WIN32
#include <windows.h>
#include <winsock.h>
#else
#define closesocket close
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#endif

#include <stdio.h>
#include <string.h>

#define PROTOPORT 5193 /* default protocol port number */
#define QLEN 6 /* size of request queue */

int visits = 0; /* counts client connections */
int nbytes;

Global variable for something so basic? Yuk. Does not give me much
confidence.

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

Old style function definition. Either it is truly ancient code (in which
case there is probably better stuff) or written by someone a long way
out of date!

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

{
struct hostent *ptrh; /* pointer to a host table entry */
struct protoent *ptrp; /* pointer to a protocol table entry */
struct sockaddr_in sad; /* structure to hold server's address */
struct sockaddr_in cad; /* structure to hold client's address */
int sd, sd2; /* socket descriptors */
int port; /* protocol port number */
int alen; /* length of address */
char buf[1000]; /* buffer for string the server sends */

#ifdef WIN32
WSADATA wsaData;
WSAStartup(0x0101, &wsaData);
#endif
memset((char *)&sad,0,sizeof(sad)); /* clear sockaddr structure */

The cast is not needed and if it was needed it would be to the wrong
type. Probably another sign of ancient code. In any case, the memset
could be used by changing the earlier definition to
struct sockaddr_in sad = {0}; /* structure to hold server's address */

sad.sin_family = AF_INET; /* set family to Internet */
sad.sin_addr.s_addr = INADDR_ANY; /* set the local IP address */

/* Check command-line argument for protocol port and extract */
/* port number if one is specified. Otherwise, use the default */
/* port value given by constant PROTOPORT */

if (argc > 1) { /* if argument specified */
port = atoi(argv[1]); /* convert argument to binary */

atoi is a bad function to use. It invokes undefined behaviour (anything
could happen) if the input value is out of range. Better to use strtol,
although you need to read up on how to use it first and remember it
returns a long.

} else {
port = PROTOPORT; /* use default port number */
}
if (port > 0) /* test for illegal value */

What about a number above 63353? (he says making use of off topic knowledge)

sad.sin_port = htons((u_short)port);

The case is not needed.

else { /* print error message and exit */
fprintf(stderr,"bad port number %s\n",argv[1]);
exit(1);

Non-portable return value. Include stdlib.h at the top then use
exit(EXIT_FAILURE);

}

/* Map TCP transport protocol name to protocol number */

if ( ((int)(ptrp = getprotobyname("tcp"))) == 0) {

Again a completely pointless cast and although it is not likely to cause
an error it is actually wrong.
if ( ((ptrp = getprotobyname("tcp"))) == 0) {
Or
if ( ((ptrp = getprotobyname("tcp"))) == NULL) {
Or
if (ptrp = getprotobyname("tcp")) {

//sprintf(buf,"This server has been contacted %d
time%s\n",visits,visits==1?".":"s.");

<snip>

Please don't use // style comments when posting to Usenet. As you can
see above they break as soon as you get line wrapping. Also, you defined
main using an implicit int, that is not supported in the only standard
that supports // style comments, so even ignoring the non-standard
headers and libraries it still would not conform.

 

[Keith Thompson]

Jens wrote, On 23/01/07 21:56: [...]

if (port > 0) /* test for illegal value */

What about a number above 63353? (he says making use of off topic knowledge)

Do you mean 65535 (2**16-1)?

[...]

Non-portable return value. Include stdlib.h at the top then use
exit(EXIT_FAILURE);

Non-portable return values are not unreasonable in non-portable code.
Judging by the non-standard (or other-standard) library routines being
used, it's likely that "exit(1)" is a correct way to indicate an
error.

On the other hand, using "exit(EXIT_FAILURE)" on such systems is
likely to yield identical results.

If the program defines (and documents!) multiple failure codes, and 1
is just one among several such codes, then using exit(1) is reasonable
(though defining and using constants would be better). If you're
just signally generic failure, exit(EXIT_FAILURE) is probably
preferred.

[...]

 

[Flash Gordon]

Keith Thompson wrote, On 24/01/07 00:17:

Jens wrote, On 23/01/07 21:56: [...]

if (port > 0) /* test for illegal value */

What about a number above 63353? (he says making use of off topic knowledge)

Do you mean 65535 (2**16-1)?

Yes. Just a tripeographical error. Although I can't see how I managed it!

[...]

Non-portable return value. Include stdlib.h at the top then use
exit(EXIT_FAILURE);

Non-portable return values are not unreasonable in non-portable code.
Judging by the non-standard (or other-standard) library routines being
used, it's likely that "exit(1)" is a correct way to indicate an
error.

On the other hand, using "exit(EXIT_FAILURE)" on such systems is
likely to yield identical results.

If the program defines (and documents!) multiple failure codes, and 1
is just one among several such codes, then using exit(1) is reasonable
(though defining and using constants would be better). If you're
just signally generic failure, exit(EXIT_FAILURE) is probably
preferred.

The only failure code used was 1. Had there been different codes for
different failures then, as you say, it would have been more reasonable.

So I basically agree with all your comments.