[vdr] thread safeness II (was: Re: Re: solved: segment faults fromosdtelext plugin)

[Rainer Zocholl <UseNet-Posting-Nospam-74308-@zocki.toppoint.de>]

While digging thru the code...

it happens that i can't resist to do a 
grep static *.c

Haven't should done that, it looks like lot of work left...



For example:
in "tools.c"  are several not thread save functions.

Some are easy to change by adding an extra parameter:

char *readline(FILE *f)
{
  static char buffer[MAXPARSEBUFFER];
  if (fgets(buffer, sizeof(buffer), f) > 0) {
     int l = strlen(buffer) - 1;
     if (l >= 0 && buffer[l] == '\n')
        buffer[l] = 0;
     return buffer; <===============
     }
  return NULL;
}


const char *strescape(const char *s, const char *chars)
{
  static char *buffer = NULL;
  const char *p = s;
  char *t = NULL;
  while (*p) {
        if (strchr(chars, *p)) {
           if (!t) {
              buffer = (char *)realloc(buffer, 2 * strlen(s) + 1);
              t = buffer + (p - s);
              s = strcpy(buffer, s);
              }
           *t++ = '\\';
           }
        if (t)
           *t++ = *p;
        p++;
        }
  if (t)
     *t = 0;
  return s; <======== is derivated from buffer, which is "(re)malloced"
            ( that will work every elegantly in single threading, but
              in multithreading two tasks overwrites or invalidates the
              memory of the other.)
}

why is buffer "static" when it contains an malloc pointer?
Is it it worth?
So why no mutex while using that buffer?
(too in the calling functions!)
or better a thread or function specific malloc.




That one looks very interessting:

int time_ms(void)
{
  static time_t t0 = 0;
  struct timeval t;
  if (gettimeofday(&t, NULL) == 0) {
     if (t0 == 0)
        t0 = t.tv_sec; // this avoids an overflow (we only work with deltas)
     return (t.tv_sec - t0) * 1000 + t.tv_usec / 1000;
     }
  return 0;
}


There must be a hint that this function MUST be called in the
init the thread free phase.
Then it seems to be OK, despite the static.
(Except that after a uptime of 2^31*10^-3 sec( approx. 24 years) 
 we will have a problem ;-))







const char *itoa(int n)
{
  static char buf[16];
  snprintf(buf, sizeof(buf), "%d", n);
  return buf;
}

add pointer parameter to callers memory.
It's not worth the risc usuing a static.


const char *AddDirectory(const char *DirName, const char *FileName)
{
  static char *buf = NULL;
  free(buf);
  asprintf(&buf, "%s/%s", DirName && *DirName ? DirName : ".", FileName);
  return buf;
}

maybe "buf" is released too early only because 
another task is using this tool too?



bool SpinUpDisk(const char *FileName)
{
  static char *buf = NULL;
 for (int n = 0; n < 10; n++) {
      free(buf);


why static? To avoid the problem that the caller must release the buffer?
No good idea in a threaded world IMHO. For single tasking that's
a beautyful idea.


const char *WeekDayName(int WeekDay)
{
  static char buffer[4];
  WeekDay = WeekDay == 0 ? 6 : WeekDay - 1; // we start with monday==0!
  if (0 <= WeekDay && WeekDay <= 6) {
     const char *day = tr("MonTueWedThuFriSatSun");
     day += WeekDay * 3;
     strncpy(buffer, day, 3);
     return buffer;  <==================!
     }
  else
     return "???";
}

const char *DayDateTime(time_t t)
{
  static char buffer[32];
  if (t == 0)
     time(&t);
  struct tm tm_r;
  tm *tm = localtime_r(&t, &tm_r);
  snprintf(buffer, sizeof(buffer), "%s %2d.%02d %02d:%02d",
WeekDayName(tm->tm_wday), tm->tm_mday, tm->tm_mon + 1, tm-
>tm_hour, tm->tm_min);
  return buffer; <====================!

}

const char *tChannelID::ToString(void)
{
  static char buffer[256];
  snprintf(buffer, sizeof(buffer), rid ? "%s-%d-%d-%d-%d" : "%s-%d-%d-%d", cSour
ce::ToString(source), nid, tid, sid, rid);
  return buffer;
}

I don't see any adavantage in using "static" in the last functions.



Even if now it is absolutely clear for the programmer never to call
that code from different tasks: There is nothing that will
inhibit that "abuse", espacially in plugins.
"But here it is working" does not proof anything in a multithreaded
environemt. Here works also everthing. When i attach the debugger...
The timing windows are so small that they may never occure on a slow
machine(because the threads have never to sleep). 

Too the problem does not show up when it is caused.
For example the above implicit "free" may release memory that is still
pointed by a task which is sleeping for 24h hours.
So the segfault may occure 24h later, but only if meanwhile nothing
alloced memory in the same area. Then that third, totally unrelated 
function may show "funny effects"...happy debugging...



tools.h contains several:

"warning returns a statically allocated string!"

that's not sufficent, as that function maybe used only by 
exactly one thread!


      

Rainer