Is this bad coding practice?

Question

I'm using PC-lint to analyze my code and theese lines are generating several errors. That makes me wonder if my coding pratice is wrong?

char *start;
char *end;

// Extract the phone number
start = (char*) (strchr(data, '\"') +1);
end = (char*) strchr(start, '\"');
*end = 0;
strlcpy((char*)Fp_smsSender, start , start-(end-1));

EDIT: After your help i now have:

char *start;
char *end;

if (data != NULL)
{
  // Extract the phone number
  start = strchr(data, '\"');
  if (start != NULL)
  {
    ++start;
    end = strchr(start, '\"');

    if (end != NULL)
    {
      *end = 0;
      strlcpy((char*)Fp_smsSender, start , FP_MAX_PHONE);
    }
  }

How does that look?

Answer 1

I imagine that what lint is complaining about is that strchr() might return a NULL pointer, and you're not checking for that before performing pointer arithmetic and dereferencing it.

You might want to do something like:

char *start;
char *end;

// Extract the phone number
start = strchr(data, '\"');
if (!start) handle_error();

++start; // skip the '\"'
end = strchr(start, '\"');
if (!end) handle_error();

*end = 0; 
strlcpy((char*)Fp_smsSender, start, size_of_Fp_smsSender_buffer);

Note that I changed the last parameter to the strlcpy() call - what that parameter is for is to specify the size of the destination buffer so you don't overrun it. The value you're passing makes no sense at all, and lint probably complained about that too. You probably meant end-(start-1) which might be more simply stated as strlen(start)+1.

Anyway, even passing in strlen(start)+1 as the last parameter to strlcpy() violates the intent of the parameter, and removes the safety strlcpy() is supposed to provide. You might as well have simply used strcpy(Fp_smsSender,start) - and if you don't know how big the Fp_smsSender destination buffer is, you should do exactly that (or fix things so you do know how big the buffer is). It'll be more clear what the code is actually doing.

Answer 2

Two things: first you don't handle NULL returns from strchr.

Second (and more seriously), the length you pass to strlcpy is wrong: you would want end - start or something similar (you have that reversed), but more fundamentally, the length argument to strlcpy should be the size of the destination buffer, not the source string.