Absturz wegen cEvent::title == NULL - Warum???

Bin mit dem Review noch nicht ganz durch, aber könnte das hier passieren?

void cEvent::SetTitle(const char *Title)
{
  title = strcpyrealloc(title, Title);
}

Wenn Title == NULL, dann kommt strcpyrealloc ebenfalls mit NULL zurück (else-Zweig):

char *strcpyrealloc(char *dest, const char *src)
{
  if (src) {
     int l = max(dest ? strlen(dest) : 0, strlen(src)) + 1; // don't let the block get smaller!
     dest = (char *)realloc(dest, l);
     if (dest)
        strcpy(dest, src);
     else
        esyslog("ERROR: out of memory");
     }
  else {
     free(dest);
     dest = NULL;
     }
  return dest;
}

Ob SetTitle irgendwo einmal mit NULL aufgerufen wird, konnte ich aus Zeitgründen noch nicht überprüfen… und muss an dieser Stelle kurz unterbrechen. Aber vielleicht weist es ja schon in die richtige Richtung.

Oh, sowas ähnliches habe ich auch beobachtet und dachte es liegt an meinem EPG-Plugin. Dort ist dann beim Event keine ChannelID eingetragen (die ich mir zum Debuggen im Skin ausgebe) und in cTimer::ToDescr() gibt es einen SIGSEGV.

Bemerkt habe ich es beim Programmieren eines Timers, wenn der externe Event durch einen DVB-Event ersetzt wird (der externe EPG reicht weiter in die Zukunft).

Während des Ersetzens ist Schedules zwar gelockt, aber meine Vermutung (Vorsicht: reine Spkulation!) ist, dass es mit cListGarbageCollector zu tun hat, aber außer Zeit fehlt mir monmentan auch ein Ansatz wie man das Debuggen kann.

Es kann natürlich auch Zufall sein, dass ich gerade dann einen Event angesehen habe, nachdem das Problem durch etwas anderes verursacht wurde...

Ich würde mal sagen, in eit.c

         pEvent = newEvent = new cEvent(SiEitEvent.getEventId());                                                                                    
         newEvent->SetStartTime(StartTime);                                                                                                          
         newEvent->SetDuration(Duration);                                                                                                            
         if (!handledExternally)                                                                                                                     
            pSchedule->AddEvent(newEvent);                                                                                                           
         }

Das Event mit title == NULL ist jetzt im Schedule. Und dann ist es eine Frage des Timings.

Wird dieses Event zuerst verwendet (mit title == NULL), oder wird zuerst

Quote

EpgHandlers.SetTitle(pEvent, ShortEventDescriptor->name.getText(buffer, sizeof(buffer)));

...

EpgHandlers.FixEpgBugs(pEvent);

aufgerufen?

Ich vermute einmal (was Klaus aber noch bestätigen sollte), dass alle Event-Strings immer mit isempty geprüft werden. Wäre es nicht dann nicht resilienter, im Initializer von cEvent alle Strings gleich mit dem leeren String zu initialisieren?

  title = "";
  shortText = "";
  description = "";

Die meisten Compiler schaffen es inzwischen ja, allen drei Zeigern die gleichen Adresse zuzuweisen. Und selbst falls nicht, wäre der Speichermehrbedarf minimal.

Und müssen wir der potentiellen Race Conditions dann trotzdem noch nachgehen?

PS: Vermutlich müsste man doch den Code daraufhin durchsuchen, wo Konstrukte wie title ? title : default verwendet werden und stattdessen konsequent auf !isempty(title) ? title : default umstellen. Auch nicht ohne Risiko…

Quote from shofmann

im Initializer von cEvent alle Strings gleich mit dem leeren String zu initialisieren?

Code

  title = "";
  shortText = "";
  description = "";

Ziemlich schlechte Idee . Diverse Methoden in VDR verlassen sich darauf, dass Pointer mit malloc alloziert wurden, z.B. strcpyrealloc .

Ups… dann müssen wir also doch die Race conditions beleuchten.

Quote from MarkusE

Ich würde mal sagen, in eit.c

Schedules und damit pSchedules ist doch eigentlich von eit.c:112 bis eit.c:437 über den WriteLock SchedulesStateKey geschützt ...

Ist euch eigentlich aufgefallen, dass wir mit den beiden von mir zitierten Code-Abschnitten:

void cEvent::SetTitle(const char *Title)
{
  title = strcpyrealloc(title, Title);
}

char *strcpyrealloc(char *dest, const char *src)
{
  if (src) {
     int l = max(dest ? strlen(dest) : 0, strlen(src)) + 1; // don't let the block get smaller!
     dest = (char *)realloc(dest, l);
     if (dest)
        strcpy(dest, src);
     else
        esyslog("ERROR: out of memory");
     }
  else {
     free(dest);
     dest = NULL;
     }
  return dest;
}

… eine – zumindest potentielle – "Use-after-free"-Vulnerability haben?

Denn expandiert entspricht dies folgendem Code:

void cEvent::SetTitle(const char *Title)
{
  char *dest = title;
  if (Title) {
     int l = max(dest ? strlen(dest) : 0, strlen(Title)) + 1;
     // vulnerability point #1: calling 'Title()' right now, another thread
     // can still access the "old" memory location to which 'title' still
     // points after its content may have been relocated to another memory
     // address and the "old" memory location that may already have been
     // released or even been reused for a different purpose
     dest = (char *)realloc(dest, l);
     if (dest)
        strcpy(dest, Title);
     else
        esyslog("ERROR: out of memory");
     }
  else {
     // vulnerability point #2: calling 'Title()' right now, another thread
     // can still access the "old" memory location to which 'title' still
     // points although its has been released or even been reused for a
     // different purpose
     free(dest);
     dest = NULL;
     }
  title = dest;
  // end of vulnerability
}

Erinnern wir uns daran, dass realloc auch einen ganz anderen Pointer als den ursprünglichen zurückliefern kann:

Quote

The realloc() function changes the size of the memory block pointed to by ptr to size bytes. The contents will be unchanged in the range from the start of the region up to the minimum of the old and new sizes. If the new size is larger than the old size, the added memory will not be initialized. If ptr is NULL, then the call is equivalent to malloc(size), for all values of size; if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr). Unless ptr is NULL, it must have been returned by an earlier call to malloc(), calloc() or realloc(). If the area pointed to was moved, a free(ptr) is done.

… ist das Ganze ist nur dann unkritisch, wenn alle Patterns der Form:

attribute = strcpyrealloc(attribute, newValue);

… immer sauber mit Semaphoren gesichert sind, sodass die Abfrage eine Attributs nicht während seines Updates erfolgen kann.

Solche Patterns treten im Code massenhaft auf (ich habe 20 Stellen gefunden) und ich kann leider nicht überblicken, ob an allen Stellen immer Locks zur Anwendung kommen. Das müsste kls beurteilen.

Würde eine Umstellung von title auf cString das Problem lösen oder lauert bei cString generell das gleiche Problem?

Super, danke dir. Hast du bei der Gelegenheit auch gleich geprüft, ob bei allen anderen Aufrufen dieser Funktion:

… Zugriffe auf die Attribute während der Ausführung sauber per Locks entkoppelt sind?

Und für die Plugins können wir ja leider an dieser Stelle keine Aussage treffen. Da muss jeder Plugin-Entwickler selbst prüfen, ob:

• bei Zugriffen alle Locks korrekt gesetzt
• die Daten nur als Kopie und nicht mit ihrem Zeiger gecacht

… werden. Sind noch andere kritische Szenarien denkbar?

Quote from shofmann

Sind noch andere kritische Szenarien denkbar?

Wie sieht es mit dem realloc() in den beiden cString::Append() in tools.c aus?

Ich habe exemplarisch mal ein wenig in Live herumgestöbert und frage mich, ob zum Beispiel hier:

EpgInfoPtr CreateEpgInfo(cSv epgid, cSchedules const *schedules)
{
  tEventID eventId = tEventID();
  tChannelID channelId = tChannelID();

  DecodeDomId(epgid, channelId, eventId);
  LOCK_CHANNELS_READ;
  cChannel const *channel = Channels->GetByChannelID(channelId);
  if (!channel) {
    return CreateEpgInfo(epgid, tr("Epg error"), tr("Wrong channel id"));
  }
  cSchedule const *schedule = schedules->GetSchedule(channel);
  if (!schedule) {
    return CreateEpgInfo(epgid, tr("Epg error"), tr("Channel has no schedule"));
  }
#if APIVERSNUM >= 20502
  cEvent const *event = schedule->GetEventById(eventId);
#else
  cEvent const *event = schedule->GetEvent(eventId);
#endif
  if (!event) {
    return CreateEpgInfo(epgid, tr("Epg error"), tr("Wrong event id"));
  }
  return CreateEpgInfo(channel, event, epgid);
}

… nicht auch noch ein LOCK_SCHEDULES_READ gebräucht würde.

Klaus, vielleicht eine kurze Hilfestellung für mich als VDR-Programmier-Newbie, der mit den folgenden Feinheiten nicht vertraut ist? Die History besagt:

Quote

There are convenience macros for easily accessing these lists without having

to explicitly set up a cStateKey and calling its Remove() function. These macros

have the form LOCK_*_READ/WRITE (with '*' being TIMERS, CHANNELS, SCHEDULES or

RECORDINGS). Simply put such a macro before the point where you need to access

the respective list, and there will be a pointer named Timers, Channels, Schedules

or Recordings, respectively, which is valid until the end of the current block.

Display More

Heißt das, dass man keine Locks setzen muss, solange man den zugeordneten Pointer (Timers, Channels, Schedules, Recordings) im Code nicht benötigt?

Andererseits besagt die History:

Quote

If a plugin needs to access several of the global lists in parallel, locking must

always be done in the sequence Timers, Channels, Recordings, Schedules. This is

necessary to make sure that different threads that need to lock several lists at

the same time don't end up in a deadlock.

Welche indirekte Abhängigkeiten gibt es diesbezüglich? Wie etwa die folgenden:

• Muss man die Channels (Sender) immer zusammen mit ihren Schedules (EPG-Daten) sperren, obwohl man nur über die Schedules iterieren möchte.
• Und müsste man den Schedule locken, wenn man über die Channels auf einen Schedule (wie in Zeile 12 oben) zugreifen möchte?
• Und wie stellt sich das Ganze dar, wenn gleichzeitig noch Timer mit dazukommen?

Fragen über Fragen…

Quote from FireFly

Wie sieht es mit dem realloc() in den beiden cString::Append() in tools.c aus?

Es gibt noch viel mehr davon:

Auch wenn die meisten davon vermutlich eher unkritisch sind, sollte man sie bei dieser Gelegenheit wohl besser doch auf potentielle Race conditions bzw. Use-after-free-Szenarien überprüfen.