Is there a problem with this code?

[creamsoda.c]

I am wondering if the following code snippet has an issue, can
somebody tell me if the parameter passed to getPacket() is done the
correct way? I expect packet to be populated with the contents. Is
this valid? I am new to C. For now please assume that the size of the
buffer passed (55) is sufficient.

extern int getPacket(unsigned char* packet);

static bool func1(message* msg)
{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

if (getPacket(packet) == REJECTED)
{
printf("FAIL: getPacket returns REJECTED\n");
return; /* no packet to read */
}

cmd = (unsigned char*) packet;
cmd_len = packet[1];


}

 

[Johannes Bauer]

static bool func1(message* msg)
{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

if (getPacket(packet) == REJECTED)

"packet" is uninitialized.

Greetings,
Johannes

 

[Walter Roberson]

static bool func1(message* msg)
{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

if (getPacket(packet) == REJECTED)

"packet" is uninitialized.

Not necessarily. The bare reference to packet devolves to
a pointer to the first element of packet, which is just
what you would want if getPacket() is passed the address
of a buffer that it fills in.

 

[Willem]

Johannes wrote:
) (e-mail address removed) schrieb:
)> static bool func1(message* msg)
)> {
)> unsigned char *cmd = NULL;
)> short cmd_len;
)> unsigned char packet[55];
)>
)> if (getPacket(packet) == REJECTED)
)
) "packet" is uninitialized.

From the looks of it, it's probably meant to be initialized by
the function getPacket.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Peter Nilsson]

(e-mail address removed) schrieb:

[ extern int getPacket(unsigned char* packet); ]

static bool func1(message* msg)
{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

The magic constant is a bit of a worry.

How does getPacket() know how much space it has?

"packet" is uninitialized.

Neither is cmd_len, but like packet, there's no
indication it should be.

Presumably getPacket() will fill packet. Of course, I'm
guessing since there's no context, but note that the
pointer parameter does not point to a constant type.

 

[Tor Rustad]

I am wondering if the following code snippet has an issue, can
somebody tell me if the parameter passed to getPacket() is done the

Without seeing the getPacket() code, who knows?

correct way? I expect packet to be populated with the contents. Is
this valid? I am new to C. For now please assume that the size of the
buffer passed (55) is sufficient.

IIRC, Ethernet header has 16 octets, IPv4 require 20 octets, or it could
be IPv6, or perhaps with a TCP header.. etc etc

extern int getPacket(unsigned char* packet);

static bool func1(message* msg)
{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

if (getPacket(packet) == REJECTED)

I prefer

if ( getPacket(packet, sizeof packet) )

or

rc = getPacket(packet, sizeof packet)
if ( REJECTED == rc )

{
printf("FAIL: getPacket returns REJECTED\n");
return; /* no packet to read */

return false;

}

cmd = (unsigned char*) packet;

why use cast here

cmd_len = packet[1];

and not here?

Have you snipped away code? 'msg' unused, and no return true

 

[Johannes Bauer]

static bool func1(message* msg)
{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

if (getPacket(packet) == REJECTED)

"packet" is uninitialized.

Not necessarily. The bare reference to packet devolves to
a pointer to the first element of packet, which is just
what you would want if getPacket() is passed the address
of a buffer that it fills in.

You (and all others) are of course right. I thought it was

void getPacket(unsigned char*)

Mea culpa!

Greetings,
Johannes

 

[Johannes Bauer]

You (and all others) are of course right. I thought it was

void getPacket(unsigned char*)

const! const! const!

Oh man. Enough making a fool out of myself for today ;-)

Greetings,
Johannes

 

[Nick Keighley]

I am wondering if the following code snippet has an issue, can
somebody tell me if the parameter passed to getPacket() is done the
correct way? I expect packet to be populated with the contents. Is
this valid?

looks ok to me

I am new to C. For now please assume that the size of the
buffer passed (55) is sufficient.

if you have any control over getPacket() I strongly suggest you
add a max_length parameter. Otherwise you may have reinvented
gets().

extern int getPacket(unsigned char* packet);

static bool func1(message* msg)

it's an unusual convention for type. I'd prefer
Message* or message_t (I'd much prefer the first one).

{
unsigned char *cmd = NULL;
short cmd_len;
unsigned char packet[55];

if (getPacket(packet) == REJECTED)
{
printf("FAIL: getPacket returns REJECTED\n");

consider writing to stderr

return; /* no packet to read */
}

cmd = (unsigned char*) packet;

no cast needed

cmd_len = packet[1];

possible alignment error or endianness problem.


cmd_len = (packet[1] << 8) | packet[2];

swop packet[1] and 2 if I have the endianess wrong.
Assumes short is 16-bits and char is 8-bits.

Sending 8-bit quantities (octets) over a comms link
is enoormously probable.


and *don't* use

cmd_len = packet[1] << 8 + packet[2];



--
Nick Keighley

In a sense, there is no such thing as a random number;
for example, is 2 a random number?
(D.E.Knuth)