`GuiLoadIcons()` names memory issues

[ZenitDS]

Hello,

Inspecting GuiLoadIcons it currently looks as follows. It appears to me as the user cannot know what the value of iconCount is, so it cannot free nor access the array safely unless the count is obtained via some other medium, which seems like not the right approach, given that the bound could be obtained here.

If the iconCount is always supposed to be RAYGUI_ICON_MAX_ICONS then I think that should be checked against iconCount. But that does not seem to be the case given the comments at the top that use iconCount as the length of the array.

I am not sure what is the way of operating in this cases, I propose adding a NULL terminator to the char ** array. This would preserve the original semantics and would not produce breakage.

Do as you prefer of course, just putting something on the thinking table

The first one would need just changing
guiIconsName = (char **)RAYGUI_CALLOC(iconCount, sizeof(char *));
with
guiIconsName = (char **)RAYGUI_CALLOC(iconCount + 1, sizeof(char *));

Cheers
-- zen

// Load raygui icons file (.rgi)
// NOTE: In case nameIds are required, they can be requested with loadIconsName,
// they are returned as a guiIconsName[iconCount][RAYGUI_ICON_MAX_NAME_LENGTH],
// WARNING: guiIconsName[]][] memory should be manually freed!
char **GuiLoadIcons(const char *fileName, bool loadIconsName)
{
    // Style File Structure (.rgi)
    // ------------------------------------------------------
    // Offset  | Size    | Type       | Description
    // ------------------------------------------------------
    // 0       | 4       | char       | Signature: "rGI "
    // 4       | 2       | short      | Version: 100
    // 6       | 2       | short      | reserved

    // 8       | 2       | short      | Num icons (N)
    // 10      | 2       | short      | Icons size (Options: 16, 32, 64) (S)

    // Icons name id (32 bytes per name id)
    // foreach (icon)
    // {
    //   12+32*i  | 32   | char       | Icon NameId
    // }

    // Icons data: One bit per pixel, stored as unsigned int array (depends on icon size)
    // S*S pixels/32bit per unsigned int = K unsigned int per icon
    // foreach (icon)
    // {
    //   ...   | K       | unsigned int | Icon Data
    // }

    FILE *rgiFile = fopen(fileName, "rb");

    char **guiIconsName = NULL;

    if (rgiFile != NULL)
    {
        char signature[5] = { 0 };
        short version = 0;
        short reserved = 0;
        short iconCount = 0;
        short iconSize = 0;

        fread(signature, 1, 4, rgiFile);
        fread(&version, sizeof(short), 1, rgiFile);
        fread(&reserved, sizeof(short), 1, rgiFile);
        fread(&iconCount, sizeof(short), 1, rgiFile);
        fread(&iconSize, sizeof(short), 1, rgiFile);

        if ((signature[0] == 'r') &&
            (signature[1] == 'G') &&
            (signature[2] == 'I') &&
            (signature[3] == ' '))
        {
            if (loadIconsName)
            {
                guiIconsName = (char **)RAYGUI_CALLOC(iconCount, sizeof(char *));
                for (int i = 0; i < iconCount; i++)
                {
                    guiIconsName[i] = (char *)RAYGUI_CALLOC(RAYGUI_ICON_MAX_NAME_LENGTH, sizeof(char));
                    fread(guiIconsName[i], 1, RAYGUI_ICON_MAX_NAME_LENGTH, rgiFile);
                }
            }
            else fseek(rgiFile, iconCount*RAYGUI_ICON_MAX_NAME_LENGTH, SEEK_CUR);

            // Read icons data directly over internal icons array
            fread(guiIconsPtr, sizeof(unsigned int), (int)iconCount*((int)iconSize*(int)iconSize/32), rgiFile);
        }

        fclose(rgiFile);
    }

    return guiIconsName;
}

[raysan5]

This can be reviewed to always initialize maximum capacity RAYGUI_ICON_MAX_ICONS

[ZenitDS]

This function seems a bit problematic in its memory handling from the untrusted input file point of view.

e.g. seems like this can read a name without the NUL terminator

guiIconsName[i] = (char *)RAYGUI_CALLOC(RAYGUI_ICON_MAX_NAME_LENGTH, sizeof(char));
fread(guiIconsName[i], 1, RAYGUI_ICON_MAX_NAME_LENGTH, rgiFile);

I have recently arrived at raylib and its peers and I am not sure wether this is by design or not. Is there somewhere I can check to see what the project goals and procedures are for future reference?