Problem with string manipulation

[aqazi]

Hi folks

I am trying to write a program which acts like a p2p server. When the
program starts it reads a file from whereit will read broadcast IP
address, port and another port number. Now I am trying skip the
comments and empty lines by saying if there is newline or # sign at the
first charecter of a line then to skip the line.But the problem is the
program is not working.

I am using the following code:

line = malloc(sizeof (char) *SIZE_OF_LINE);

while(fgets(line, SIZE_OF_LINE + 1, fp) != NULL)
{
i f(line[0] != '#' || line[0] != '\n')
{
fprintf(stderr,"i am here\n");
fprintf(stderr,"%s", line);
strcpy(tmpEntry, strtok(line, ":"));
strcpy(tmpValue, strtok(NULL, "\n"));
fprintf(stderr,"\t%s", tmpEntry);
fprintf(stderr,"\t: %s\n", tmpValue);
}
}

and the file the program is reading is:

################################################## ###########################
# P2P Conf File
#Please Don't modify this file as this is file is used to
#create all required sockets
# Author : Ahetesham Qazi
################################################## ###########################

Brodcast_IP:192.168.1.255
Brodcast_Port:30001
Comm_Port:30101

The program should skip the first seven lines but the problem it's not
skipping. and when it's trying to tokenize the first line and then
printing it it's giving me segmentation fault.

Any suggetion would be greatly appreciated
Thanks
Ahetesham Qazi

[Richard Heathfield]

aq***@inbox.com said:

if(line[0] != '#' || line[0] != '\n')

How many characters can you think of that will fail this condition?

Read it out loud. If this character is not a hash, OR it is not a newline...

So a hash PASSES the test because it is not a newline, and a newline PASSES
the test because it is not a hash.

You meant to use && rather than ||

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

[Gordon Burditt]

>I am trying to write a program which acts like a p2p server. When the

>program starts it reads a file from whereit will read broadcast IP
address, port and another port number. Now I am trying skip the
comments and empty lines by saying if there is newline or # sign at the
first charecter of a line then to skip the line.

....

>I am using the following code:

line = malloc(sizeof (char) *SIZE_OF_LINE);

while(fgets(line, SIZE_OF_LINE + 1, fp) != NULL)
{
i f(line[0] != '#' || line[0] != '\n')

A newline is not equal to '#'.
A # is not equal to '\n'.
Another character is not equal to either.
In other words, the condition is always true.

Do you really want to use OR here?

[aqazi]

Richard Heathfield wrote:

aq***@inbox.com said:

if(line[0] != '#' || line[0] != '\n')

How many characters can you think of that will fail this condition?

Read it out loud. If this character is not a hash, OR it is not a newline...

So a hash PASSES the test because it is not a newline, and a newline PASSES
the test because it is not a hash.

You meant to use && rather than ||

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

Thanks for the solution. Actually this is a strictly predefined file.
So whithout the actual data
the only charecter can be at the start of the file is "#" or "\n"
Ahetesham Qazi

[Richard Heathfield]

aq***@inbox.com said:

>
Richard Heathfield wrote:

>aq***@inbox.com said:

if(line[0] != '#' || line[0] != '\n')

How many characters can you think of that will fail this condition?

Read it out loud. If this character is not a hash, OR it is not a
newline...

So a hash PASSES the test because it is not a newline, and a newline
PASSES the test because it is not a hash.

You meant to use && rather than ||

Thanks for the solution. Actually this is a strictly predefined file.
So whithout the actual data
the only charecter can be at the start of the file is "#" or "\n"

Nevertheless, all characters, *including* hash and newline, will pass your
existing test, the one that is intended to filter them out. When you change
the || to && the filter will have the desired effect.
--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

[Nils O. Selåsdal]

aq***@inbox.com wrote:

Hi folks

I am trying to write a program which acts like a p2p server. When the
program starts it reads a file from whereit will read broadcast IP
address, port and another port number. Now I am trying skip the
comments and empty lines by saying if there is newline or # sign at the
first charecter of a line then to skip the line.But the problem is the
program is not working.

I am using the following code:

line = malloc(sizeof (char) *SIZE_OF_LINE);

while(fgets(line, SIZE_OF_LINE + 1, fp) != NULL)

Do not lie to fgets. You only have storage for
SIZE_OF_LINE chars, not SIZE_OF_LINE + 1.

[Nick Keighley]

aq***@inbox.com wrote:

line = malloc(sizeof (char) *SIZE_OF_LINE);

sizeof(char) is by definition 1, *always*, so this can be reduced to

line = malloc (SIZE_OF_LINE);
--
Nick Keighley

[Richard Heathfield]

Nick Keighley said:

aq***@inbox.com wrote:

>line = malloc(sizeof (char) *SIZE_OF_LINE);

sizeof(char) is by definition 1, *always*, so this can be reduced to

line = malloc (SIZE_OF_LINE);

That's one perfectly sensible approach. Here's another:

line = malloc(SIZE_OF_LINE * sizeof *line);

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

[Ancient_Hacker]

aq***@inbox.com wrote:

line = malloc(sizeof (char) *SIZE_OF_LINE);

This would be a bit safer as: line = malloc( sizeof( *line ) );

while(fgets(line, SIZE_OF_LINE + 1, fp) != NULL)

similarly: while(fgets(line, sizeof( *line ), fp) != NULL)

if(line[0] != '#' || line[0] != '\n')

Several things amiss here:

(1) Are you sure if line[0] has been set? Yes, fgets() USUALLY puts
in a "\n", but not in every case.
You should do something like this first: : if( strlen(line) 0 ) {
....

(2) The tests are backwards.. It's more obvious if you write:

if( line[0] == '#' || line[0] == '\n' ) { /* ignore this line */ }
else { /* process it */
...
}

(3) What if the user accidentally types a space or tab, either leading
or trailing the line? Wouldnt hurt to handle these cases too.
(3) You're being awfully optimistic about what's in the file. I'd add
several tests to ensure that strtok finds what you expect, and the
length of the token doesnt overflow the destination. What you have
right now is an excellent way for somebody to crash or own your server
with a buffer overflow if they get write access to this file.

[Richard Heathfield]

Ancient_Hacker said:

>
aq***@inbox.com wrote:

>line = malloc(sizeof (char) *SIZE_OF_LINE);

This would be a bit safer as: line = malloc( sizeof( *line ) );

Er, no it wouldn't.

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

[Barry Schwarz]

On 5 Oct 2006 04:10:04 -0700, "Ancient_Hacker" <gr**@comcast.net>
wrote:

>
aq***@inbox.com wrote:

>line = malloc(sizeof (char) *SIZE_OF_LINE);

This would be a bit safer as: line = malloc( sizeof( *line ) );

Since line appears to be a char* (see your note (2) below), this
allocates one byte. Safer perhaps but not usable.

>

>while(fgets(line, SIZE_OF_LINE + 1, fp) != NULL)

similarly: while(fgets(line, sizeof( *line ), fp) != NULL)

Why read only 1 byte of the line?

>

> if(line[0] != '#' || line[0] != '\n')

Several things amiss here:

(1) Are you sure if line[0] has been set? Yes, fgets() USUALLY puts
in a "\n", but not in every case.

It is pretty rare for fgets to put a \n in the first position of a
buffer. Usually happens when the line is empty.

>You should do something like this first: : if( strlen(line) 0 ) {
...

(2) The tests are backwards.. It's more obvious if you write:

if( line[0] == '#' || line[0] == '\n' ) { /* ignore this line */ }
else { /* process it */
...
}

(3) What if the user accidentally types a space or tab, either leading
or trailing the line? Wouldnt hurt to handle these cases too.
(3) You're being awfully optimistic about what's in the file. I'd add
several tests to ensure that strtok finds what you expect, and the
length of the token doesnt overflow the destination. What you have
right now is an excellent way for somebody to crash or own your server
with a buffer overflow if they get write access to this file.

Remove del for email

[Ancient_Hacker]

Richard Heathfield wrote:

Ancient_Hacker said:

aq***@inbox.com wrote:

line = malloc(sizeof (char) *SIZE_OF_LINE);

This would be a bit safer as: line = malloc( sizeof( *line ) );

Er, no it wouldn't.

Note to self: engage brain before typing.

What I shudda typed:

In general it's safer to use quantities that are closely correlated.

For example:

#define SZ 1000

typedef char String[ SZ ];
typedef String * StringPtr;

StringPtr p;

p = (StringPtr) malloc( sizeof( char ) * SZ ); // this is
correct, but a bit indirect

p = (StringPtr) malloc( sizeof( *p ) ); // this is a
whole lot more direct.

.... In the first malloc, everything is hunky-dorey, until soembody
changes the type of the string to 2-byte characters, then things go
blooey. Or someone changes SZ to StringSZ in the first two occurences,
but not the one in the malloc.

.... the second malloc is considerably less prone to blowing up, as
we're using the size of what p points to to initialize "p".

Now you can't always do this, and it does take a bit more typing and
static typedefs, but what price safety?

[Richard Heathfield]

Ancient_Hacker said:

<snip>

>
#define SZ 1000

typedef char String[ SZ ];

EEK!! Naming an array of a particular (and rather low) size "String" is just
asking to confuse people.

String x; /* x is a String, but not a string! */

char foo[] = "I'm a string"; /* foo holds a string, but not a String! */

typedef String * StringPtr;

AARGH!! Hiding a pointer in a typedef!

StringPtr p;

p = (StringPtr) malloc( sizeof( char ) * SZ ); // this is
correct, but a bit indirect

The cast is unnecessary.

p = (StringPtr) malloc( sizeof( *p ) ); // this is a
whole lot more direct.

True, but the cast is still unnecessary.

<snip>

Now you can't always do this, and it does take a bit more typing and
static typedefs, but what price safety?

I prefer, where possible, to get my safety without compromising generality.

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at above domain (but drop the www, obviously)

[Ancient_Hacker]

Richard Heathfield wrote:

Ancient_Hacker said:

<snip>

#define SZ 1000

typedef char String[ SZ ];

EEK!! Naming an array of a particular (and rather low) size "String" is just
asking to confuse people.

>
AARGH!! Hiding a pointer in a typedef!

Sorry to see my conventions alarm you. Guess we'll have to disagree as
to what's best.

I prefer, where possible, to get my safety without compromising generality.

I prefer to have safety first, even if the straight-jacket chafes a
bit.

One can always resort to rambunction like: p = malloc( sizeof( foo ) *
bar ) if there is no alternative.

What really frosts my fritters is seeing lines like "bar = 32 + 400 +
4" when it really should be something like bar = sizeof( struct a ) +
sizeof( LongArrayType ) + sizeof( long );

Way too much of this code out there.

[Keith Thompson]

"Ancient_Hacker" <gr**@comcast.netwrites:

Richard Heathfield wrote:

>Ancient_Hacker said:
<snip>

>
#define SZ 1000

typedef char String[ SZ ];

EEK!! Naming an array of a particular (and rather low) size "String" is just
asking to confuse people.

AARGH!! Hiding a pointer in a typedef!

Sorry to see my conventions alarm you. Guess we'll have to disagree as
to what's best.

The typedef in question was
typedef String * StringPtr;

The fact that the name ends in "Ptr" means it's not *too* bad, but
using a typedef for a pointer still makes me uneasy. I would drop the
typedef and just use "String *" wherever you'd use "StringPtr".

For that matter, I probably wouldn't use a pointer to an array. It's
perfectly legal, of course, but I think a pointer to the first element
of the array is more idiomatic and more general. (If I really wanted
a pointer to the full array, I'd probably wrap it in a structure.)

And I certainly wouldn't use the name "String", for reasons I think
Richard has already discussed.

>I prefer, where possible, to get my safety without compromising generality.

I prefer to have safety first, even if the straight-jacket chafes a
bit.

One can always resort to rambunction like: p = malloc( sizeof( foo ) *
bar ) if there is no alternative.

Or, better:

p = malloc(bar * sizeof *p);

I'm assuming that bar is a count. I'm also assuming that p is of type
foo, something that I don't have to assume with the improved form.

What really frosts my fritters is seeing lines like "bar = 32 + 400 +
4" when it really should be something like bar = sizeof( struct a ) +
sizeof( LongArrayType ) + sizeof( long );

Way too much of this code out there.

Agreed wholeheartedly.

--
Keith Thompson (The_Other_Keith) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <* <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.