C - Why is my function returning NULL?

Question

I think my function is returning NULL since I initilize it to it. But I get compiling error if I dont.

This is just a prototype that I made in a test.c file to test it. So when I get it to work I will copy back the lookup function back into the correct file.

This is part of pset6 of cs50 if that helps anyone.

const char* lookup(const char* extension);

int main(void)
{
    const char* type = "css";
    const char* ending = lookup(type);  
    printf("the exstension: %s\nis of type = %s\n", type, ending);
}

const char* lookup(const char* extension)
{

    char temp[strlen(extension)];

    for (int i = 0; i < strlen(temp); i++)
    {
        if (isalpha(extension[i]))
            temp[i] = tolower(extension[i]);
    }

    printf("temp = %s\n", temp);

    char* filetype = NULL;

    if (strcmp(temp,  "html") == 0)
        strcpy(filetype, "text/html"); 

    else if(strcmp(temp, "css") == 0)
        strcpy(filetype, "text/css");

    else if(strcmp(temp, "js") == 0)
        strcpy(filetype, "text/js");

    else if(strcmp(temp, "jpg") == 0)
        strcpy(filetype, "image/jpg");

    else if(strcmp(temp, "ico" ) == 0)
        strcpy(filetype, "image/x-icon");

    else if(strcmp(temp, "gif") == 0)
        strcpy(filetype, "image/gif");

    else if(strcmp(temp, "png") == 0)
        strcpy(filetype, "image/png");

    else
        return NULL;

    return filetype;
}

I'm using all the correct libraries, it screwed up my code preview when I tried to include them!

@BLUEPIXY Im trying to reduce the amount of `return`s since i think its bad pratice (correct me if im wrong) to have many of them. And i can't assign a string into a variable since thats just wrong.. I need to copy it. — mrfr, Oct 30 '15 at 11:24

score 4 · Accepted Answer · answered Oct 30 '15 at 10:57

4

 char temp[strlen(extension)];

You don't reserve the space for the trailing null character and you never set it! For example, char temp[strlen(extension) + 1] = {0};.

Then:

char* filetype = NULL;

if (strcmp(temp,  "html") == 0)
    strcpy(filetype, "text/html");

filetype pointed object must be allocated, for example using malloc, otherwise strcpy is copying with a null pointer.

answered Oct 30 '15 at 10:57

ouah

142,963
15
272
331

Thx! But what do you mean with "otherwise `strcpy` is copying with a null pointer." ? How does `malloc` determain if it is copied with/without the `null` pointer? – mrfr Oct 30 '15 at 11:15
And also, why do i have to set `temp[strlen(extension) + 1]` to `{0};` ? – mrfr Oct 30 '15 at 11:17
To ensure the trailing null character is set in your for loop . – ouah Oct 30 '15 at 11:30

i486 · Answer 2 · 2015-10-30T11:13:29.620

Are you sure that extension contains only extension without .? I can prefer to use _stricmp, strcmpi to compare case insensitive. And why you do strcpy to filetype instead of assignment? You have only pointer without malloc:

const char* lookup(const char* extension)
{
const char* filetype = NULL;

if (_stricmp(extension, "html") == 0)
    filetype = "text/html"; 
else if(_stricmp(extension, "css") == 0)
    filetype = "text/css";

else if(_stricmp(extension, "js") == 0)
    filetype = "text/js";

else if(_stricmp(extension, "jpg") == 0)
    filetype = "image/jpg";

else if(_stricmp(extension, "ico" ) == 0)
    filetype = "image/x-icon";

else if(_stricmp(extension, "gif") == 0)
    filetype = "image/gif";

else if(_stricmp(extension, "png") == 0)
    filetype = "image/png";

return filetype;
}

Or better:

const char* lookup(const char* extension)
{
  char * ext[] = { "html", "text/html", "css", "text/css", "js", "text/js", "jpg", "image/jpg", NULL };


  for ( int i = 0; ext[i]; i += 2 )
  {
    if ( !stricmp( extension, ext[i] ) )
      return ext[i+1];
  }
  return NULL;
}

What is `_stricmp()` and where does it come from. It isn't Standard C nor POSIX. — alk, Oct 30 '15 at 11:10
Ought to be `const char* filetype`; string literal-to-`char*` is deprecated, and a bad idea anyways. — , Oct 30 '15 at 11:12

score 1 · Answer 3 · answered Oct 30 '15 at 10:58

1

Be careful with this:

char temp[strlen(extension)];

In C, strings are NULL terminated, so you actually do not reserve space for the terminating character, so your temp string might actually look much longer at runtime.

Do this instead:

char temp[strlen(extension)+1];

And later on:

temp[i] = '\0';

answered Oct 30 '15 at 10:58

SirDarius

41,440
8
86
100

Could you explain further on the `temp[i] = '\0';` part? – mrfr Oct 30 '15 at 11:28
The '\0' is a C convention to signal the end of string. Every str* function expects it as a delimiter. – SirDarius Oct 30 '15 at 12:15

score 1 · Answer 4 · answered Oct 30 '15 at 10:58

1

You must allocate room for the terminating null and terminate the string:

char temp[strlen(extension)+1];

for (int i = 0; i < strlen(temp); i++)
{
    if (isalpha(extension[i]))
        temp[i] = tolower(extension[i]);
}
temp[i]= '\0';

Note also that if the extension contains a digit or any other non-alpha character, it will not be copied.

answered Oct 30 '15 at 10:58

Paul Ogilvie

25,048
4
23
41

Yes im 99% sure that i dont have a non-alphabetical character. Since later in the **distribution** code. They check for a php ending like `"php"` – mrfr Oct 30 '15 at 11:29

OznOg · Answer 5 · 2015-10-31T12:31:16.510

You lack the trailing '\0' extra room in the temp; you need to write something like

char temp[strlen(extension) + 1];

and to allocate some space where to store the filetype; probably by writing

char filetype[50]; // 50 should be enought for your case

Nevertheless, I would suggest using strcasecmp() (function compares the two strings, ignoring the case of the characters) instead of strcmp() and remove the filetype which seems useless. Here what it could look like:

#include <stdio.h>
#include <strings.h>

const char *lookup(const char *extension);

int main(void)
{
    const char *const type = "css";
    const char *ending = lookup(type);
    printf("the exstension: %s\nis of type = %s\n", type, ending);
}

const char *lookup(const char *extension)
{
    if (strcasecmp(extension, "html") == 0)
        return "text/html";

    else if (strcasecmp(extension, "css") == 0)
        return "text/css";

    else if (strcasecmp(extension, "js") == 0)
        return "text/js";

    else if (strcasecmp(extension, "jpg") == 0)
        return "image/jpg";

    else if (strcasecmp(extension, "ico" ) == 0)
        return "image/x-icon";

    else if (strcasecmp(extension, "gif") == 0)
        return "image/gif";

    else if (strcasecmp(extension, "png") == 0)
        return "image/png";

    return NULL;
}

A more scalable solution could use an array to describe extensions, so that you would not need to change the code if you add a new type:

#include <stdio.h>
#include <strings.h>

struct Type {
    const char *const extension;
    const char *const mime;
} knownTypes[] = {
    { "html", "text/html"    },
    { "css",  "text/css"     },
    { "js",   "text/js"      },
    { "jpg",  "image/jpg"    },
    { "ico",  "image/x-icon" },
    { "gif",  "image/gif"    },
    { "png",  "image/png"    }
};

static const size_t nbKnownTypes = sizeof(knownTypes) / sizeof(struct Type);

const char* lookup(const char* extension);

int main(void)
{
    const char *const type = "Css";
    const char *ending = lookup(type);
    printf("the exstension: %s\nis of type = %s\n", type, ending);
}

const char *lookup(const char *extension)
{
    for (size_t i = 0; i < nbKnownTypes; i++) {
         struct Type type = knownTypes[i];
         if (strcasecmp(extension, type.extension) == 0)
             return type.mime;
    }

    return "Unknown mime type";
}

Whith this kind of setup, you may just add the extension and the mime type of a new type easily (You could put this stucture in a separate c file, that could prevent recompiling everything as well, but that is an other story)

ameyCU · Answer 6 · 2015-10-30T11:20:26.417

1. Your code exhibits undefined behaviour . In your function lookup-

char temp[strlen(extension)];     // basically char temp[3]

and with loop you fill full array , without leaving space for '\0' and then print using %s and passing it to strcmp will also cause UB.

Declare you array temp like this -

char temp[strlen(extension)+1)]={'\0'};        // +1 for null character

2. Also when you copy in pointer filetype-

if (strcmp(temp,  "html") == 0)
    strcpy(filetype, "text/html");

But it points to NULL as it is not allocated any memory.

Allocate memory to filetype using malloc.

score -1 · Answer 7 · edited Oct 31 '15 at 08:53

-1

char* filetype = NULL does not have memory space to copy string using strcpy function. Replace this code by char* filetype = malloc(20).

edited Oct 31 '15 at 08:53

IKavanagh

6,089
11
42
47

answered Oct 30 '15 at 11:40

Nayan Khant

1
2

C - Why is my function returning NULL?

7 Answers7