calls to printf seem to overwrite array of strings

Question

Hello I am developing a program for the Raspberry in C (the in-progress project can be found here).

I noted there are some errors in the task1 function so I created an equivalent program in my Desktop (running Ubuntu) to find the error, where the task1 was readapted as below:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "stub.h"
#include "globals.h"
#include "utils.h"
#include <pthread.h>
#define INPIN 25
void takePic(char picname[24]);

//This thread reads the PIR sensor output
void task1()
{
    unsigned char val_read = 0;
    static unsigned char alarm_on = FALSE;
    static const unsigned int maxNumPics=10;
    static char folderPath[] = "/home/usr/Documents/alarmSys_rasp/alarm_new/pics/";
    char fileNameTodelete[73];
    fileNameTodelete[0] = '\0';
    strcat(fileNameTodelete, folderPath);
    //INITIALIZING
    pinMode(INPIN, INPUT);
    //create folder where to save pics
    createFolder(folderPath);

    char* names[24];
    char picname[24];
    int res = 0;
    picname[0] = '\0';
    static unsigned int numPicsInFolder;
    //delete if more than 10 files
    while((numPicsInFolder =  filesByName(names, folderPath))>maxNumPics)
    {

        fileNameTodelete[0] = '\0';
        strcat(fileNameTodelete, folderPath);
        strcat(fileNameTodelete, names[0]);
        printf("%s\n", fileNameTodelete);
        remove(fileNameTodelete);
    }

    static unsigned int nexEl;
    nexEl = numPicsInFolder % maxNumPics;
    printf("Entering while\n");
    while(1)
    {
        //static const unsigned int del = 300;

        val_read = digitalRead(INPIN);
        if (val_read && (!alarm_on)) //motion detected
        {
            printf("\nDetected movement\n");
            if (numPicsInFolder >= maxNumPics)
            {
                printf("\nMax num pics\n");
                fileNameTodelete[0] = '\0';
                strcat(fileNameTodelete, folderPath);
                strcat(fileNameTodelete, names[nexEl]);
                printFiles(names, numPicsInFolder);
                printf("File to be deleted %d: %s, ", nexEl, names[nexEl]);
                //printf("%s\n", fileNameTodelete);

                if ((res = remove(fileNameTodelete))!=0)
                {
                    printf("Error deleting file: %d\n", res);
                }

            }
            else
            {
                printf("\nNot reached max num pics\n");
                numPicsInFolder++;
            }
            //update buffer
            takePic(picname);
            printf("value returned by takePic: %s\n", picname);
            //names[nexEl] = picname;
            strcpy(names[nexEl], picname); //ERROR HERE
            printFiles(names, numPicsInFolder);

            printf("curr element %d: %s\n",nexEl, names[nexEl]);

            nexEl++;
            nexEl %= maxNumPics;
            printf("\nDetected movement: alarm tripped\n\n");
            alarm_on = TRUE;
            /*Give some time before another pic*/
        }
        else if (alarm_on && !val_read)
        {
            alarm_on = FALSE;
            printf("\nAlarm backed off\n\n");
        }
    }

}


void takePic(char picname[24])
{
    /*Build string to take picture*/
    int err;
    //finalcmd is very long
    char finalcmd[150];
    finalcmd[0] = '\0';
    getDateStr(picname);

    char cmd1[] = "touch /home/usr/Documents/alarmSys_rasp/alarm_new/pics/";
    char cmdlast[] = "";


    strcat(finalcmd, cmd1);
    strcat(picname, ".jpg");
    strcat(finalcmd, picname);
    strcat(finalcmd, cmdlast);
    system(finalcmd);

    if ((err=remove("/var/www/html/*.jpg"))!=0)
    {
        printf("Error deleting /var/www/html/*.jpg, maybe not existing\n" );
    }
    //system(finalcmd_ln);
    //pthread_mutex_lock(&g_new_pic_m);
    g_new_pic_flag = TRUE;
    printf("\nPicture taken\n\n");

}

DESCRIPTION

The main function calls the task1 function defined in the file task1.c. The function creates a file in the folder ./pics/ every time the condition (val_read && (!alarm_on)) is verified (in the simulation this condition is satisfied every 2 loops). The function allows only 10 files in the folder. If there are already 10, it deletes the oldest one and creates the new file by calling the function takePic.

The name of files are stored in a array of strings char* names[24]; and the variable nexEl points to the element of this array having the name of the oldest file so that it is replaced with the name of the new file just created.

PROBLEM

The problem is the following: the array char* names[24] is correctly populated at the first iteration but already in the second iteration some elements are overwritten. The problem arises when the folder has the maximum number of files (10) maybe on the update of the array. It seems the calls to printf overwrite some of its elements so that for example one of them contains the string "Error deleting /var/www/html/*.jpg, maybe not existing\n" printed inside the funtion takePic.

What am I missing or doing wrong with the management of arrays of strings?

UTILITIES FUNCTIONS

To be complete here are shortly described and reported other functions used in the program. The function getDateStr builds a string representing the current date in the format yyyy_mm_dd_hh_mm_ss.

The function filesByName builds an array of strings where each string is the name of a file in the folder ./ ordered from the last file created to the newest.

The function printFiles prints the previous array.

void getDateStr(char str[20])
{
    char year[5], common[3];

    time_t t = time(NULL);
    struct tm tm = *localtime(&t);
    str[0]='\0';
    sprintf(year, "%04d", tm.tm_year+1900);
    strcat(str, year);
    sprintf(common, "_%02d", tm.tm_mon + 1);

    strcat(str, common);

    sprintf(common, "_%02d", tm.tm_mday);
    strcat(str, common);

    sprintf(common, "_%02d", tm.tm_hour);
    strcat(str, common);

    sprintf(common, "_%02d", tm.tm_min);
    strcat(str, common);

    sprintf(common, "_%02d", tm.tm_sec);
    strcat(str, common);
    //printf("%s\n", str);

}

unsigned int countFiles(char* dir)
{
    unsigned int file_count = 0;
    DIR * dirp;
    struct dirent * entry;
    dirp = opendir(dir); /* There should be error handling after this */
    while ((entry = readdir(dirp)) != NULL) {
        if (entry->d_type == DT_REG) { /* If the entry is a regular file */
             file_count++;

        }
    }
    return file_count;

}

void printFiles(char* names[24], unsigned int file_count)
{
    for (int i=0; i<file_count; i++)
    {
        printf("%s\n", names[i]);
    }
}

unsigned int filesByName(char* names[24], char* dir)
{
    unsigned int file_count = 0;
    DIR * dirp;
    struct dirent * entry;
    dirp = opendir(dir); /* There should be error handling after this */

    while ((entry = readdir(dirp)) != NULL) {
        if (entry->d_type == DT_REG) { /* If the entry is a regular file */
             //strncpy(names[file_count], entry->d_name,20);
            //names[file_count] = malloc(24*sizeof(char));
            names[file_count] = entry->d_name;
             file_count++;
        }
    }
    closedir(dirp);
    char temp[24];
    if (file_count>0)
    {
        for (int i=0; i<file_count-1; i++)
        {
            for (int j=i; j<file_count; j++)
            {
                if (strcmp(names[i], names[j])>0)
                {
                    strncpy(temp, names[i],24);
                    strncpy(names[i], names[j],24);
                    strncpy(names[j], temp, 24);
                    }
            }
        }
    }
    return file_count;
}

For the simulation I created also the following function (digitalRead is actually a function of the wiringPi C library for the Raspberry):

int digitalRead(int INPIN)
{
    static int res = 0;
    res = !res;
    return res;
}

Answer 1

getDateStr() uses a char buffer that is always too small. Perhaps other problems exists too.

void getDateStr(char str[20]) {
  char year[5], common[3];
  ....
  sprintf(common, "_%02d", tm.tm_mon + 1);  // BAD, common[] needs at least 4

Alternative with more error checking

char *getDateStr(char *str, size_t sz) {
  if (str == NULL || sz < 1) {
    return NULL;
  }
  str[0] = '\0';

  time_t t = time(NULL);
  struct tm *tm_ptr = localtime(&t);
  if (tm_ptr == NULL) {
    return NULL;
  }    
  struct tm tm = *tm_ptr;

  int cnt = snprintf(year, sz, "%04d_%02d_%02d_%02d_%02d_%02d", 
      tm.tm_year+1900, tm.tm_mon + 1, tm.tm_mday,
      tm.tm_hour, tm.tm_min, tm.tm_sec);
  if (cnt < 0 || cnt >= sz) {
    return NULL;
  }
  return str;
}

Answer 2

In task1, you have char *names[24]. This is an array of char pointers.

In filesByName, you do

names[file_count] = entry->d_name;

but should be doing

names[file_count] = strdup(entry->d_name);

because you can't guarantee that d_name persists or is unique after the function returns or even within the loop. You were already close with the commented out malloc call.

Because you call filesByName [possibly] multiple times, it needs to check for names[file_count] being non-null so it can do a free on it [to free the old/stale value from a previous invocation] before doing the strdup to prevent a memory leak.

Likewise, in task1,

strcpy(names[nexEl], picname); //ERROR HERE

will have similar problems and should be replaced with:

if (names[nexEl] != NULL)
    free(names[nexEl]);
names[nexEl] = strdup(picname);

There may be other places that need similar adjustments. And, note that in task1, names should be pre-inited will NULL

---

Another way to solve this is to change the definition of names [everywhere] from:

char *names[24];

to:

char names[24][256];

This avoids some of the malloc/free actions.