Rainer Zocholl wrote:

Again: The most beloved problem: Reentrancy...

This C-Function in tools.c looks suspicious:

cString TimeToString(time_t t) { char buffer[32]; if (ctime_r(&t, buffer)) { buffer[strlen(buffer) - 1] = 0; // strip trailing newline return buffer; } return "???"; }

It's usually no good idea to deference a variable on the stack OUTSIDE the function it self..the next call to an other functiom may overwrite the values! It might work very often, but it is not really much better than a "static"!

Is there really no tool like "bounds checker for linux" that will warn at those obvious booby traps?

Having a deja vue: I thought all that was "overworked" with the "thread-save" patches 3 month ago?

Or didn't these patches made the way into VDR?

The fastest fix may be: Make it like in ctime_r an define a buffer pint as parameter. The slowest: use the ugly time wasting asprintf and never forget to free the memory...

cString creates a copy of buffer upon return.

Klaus

linux@youmustbejoking.demon.co.uk(Darren Salt) 01.06.05 20:28

I demand that Rainer Zocholl may or may not have written...

...

Again: The most beloved problem: Reentrancy...

...

This C-Function in tools.c looks suspicious:

...

cString TimeToString(time_t t) { char buffer[32]; if (ctime_r(&t, buffer)) { buffer[strlen(buffer) - 1] = 0; // strip trailing newline return buffer; } return "???"; }

That's harmless.

On the first view it does not look so ;-)

Those return statements are effectively 'return cString (<string>, false)', and the cString constructor will call strdup() if its second parameter is false (and note that that parameter is declared as having a default value).

Ah, ok, but's not vey effective to copy reach string serveral times IMHO.

Pardon my stupid question: And who is freeing that malloced memory later? for example:

cTDT::cTDT(const u_char *Data) :SI::TDT(Data, false) { CheckParse();

time_t sattim = getTime(); time_t loctim = time(NULL);

if (abs(sattim - loctim) > 2) { mutex.Lock(); isyslog("System Time = %s (%ld)\n", *TimeToString(loctim),loctim); isyslog("Local Time = %s (%ld)\n", *TimeToString(sattim),sattim); if (stime(&sattim) < 0) esyslog("ERROR while setting system time: %m"); mutex.Unlock(); } }

The pointer is not stored anywhere. (At least not obviously)

cString strescape(const char *s, const char *chars) { char *buffer; const char *p = s; char *t = NULL; while (*p) { if (strchr(chars, *p)) { if (!t) { buffer = MALLOC(char, 2 * strlen(s) + 1); t = buffer + (p - s); s = strcpy(buffer, s); } *t++ = '\'; } if (t) *t++ = *p; p++; } if (t) *t = 0; return cString(s, t != NULL); }

What happens if the malloc fails? VDR will coredump because of the "*t++" by intention?

Pardon again the stupid question: where/how is that memory freed? recording.c:

void cRecordingUserCommand::InvokeCommand(const char *State, const char *RecordingFileName) { if (command) { char *cmd; asprintf(&cmd, "%s %s "%s"", command, State, *strescape(RecordingFileName, ""$")); isyslog("executing '%s'", cmd); SystemExec(cmd); free(cmd); } }

Does asprintf know it can release the memory? How?

Rainer

I demand that Rainer Zocholl may or may not have written...

Darren Salt 01.06.05 20:28

...

I demand that Rainer Zocholl may or may not have written...

...

Again: The most beloved problem: Reentrancy...

...

...

This C-Function in tools.c looks suspicious:

...

...

cString TimeToString(time_t t) { char buffer[32]; if (ctime_r(&t, buffer)) { buffer[strlen(buffer) - 1] = 0; // strip trailing newline return buffer; } return "???"; }

...

That's harmless.

On the first view it does not look so ;-)

True. That's one of the features of C++ ;-)

...

Those return statements are effectively 'return cString (<string>, false)', and the cString constructor will call strdup() if its second parameter is false (and note that that parameter is declared as having a default value).

Ah, ok, but's not very effective to copy each string several times IMHO.

You have something which is guaranteed to be freeable by the destructor...

Pardon my stupid question: And who is freeing that malloced memory later?

The caller, as soon as the object goes out of scope (the compiler will automatically insert a call to the object's destructor at that point).

for example:

cTDT::cTDT(const u_char *Data) :SI::TDT(Data, false) {

[snip]

 isyslog("System Time = %s (%ld)\n", *TimeToString(loctim),loctim);
 isyslog("Local Time  = %s (%ld)\n", *TimeToString(sattim),sattim);

[snip; no obvious free or delete]

}

The pointer is not stored anywhere. (At least not obviously)

There's no visible pointer: TimeToString() returns cString, not cString*. What looks like a dereference is really cString::operator*().

The object is in temporary storage somewhere. It goes out of scope just after the isyslog() call is completed.

cString strescape(const char *s, const char *chars) { char *buffer; const char *p = s; char *t = NULL; while (*p) { if (strchr(chars, *p)) { if (!t) { buffer = MALLOC(char, 2 * strlen(s) + 1); t = buffer + (p - s);

[snip]

return cString(s, t != NULL); }

What happens if the malloc fails? VDR will coredump because of the "*t++"

That is likely, but it could happen even if the malloc succeeded if the system is really short of memory - though this depends on the overcommit setting, as exposed in /proc/sys/vm/overcommit_memory or the equivalent sysctl. See the kernel documentation for more information.

by intention?

Probably not :-)

Pardon again the stupid question: where/how is that memory freed?

In this case, when the cString object is deleted - have a look at its constructor and destructor.

recording.c:

void cRecordingUserCommand::InvokeCommand(const char *State, const char *RecordingFileName) { if (command) { char *cmd; asprintf(&cmd, "%s %s "%s"", command, State, *strescape(RecordingFileName, ""$")); isyslog("executing '%s'", cmd); SystemExec(cmd); free(cmd); } }

Does asprintf know it can release the memory?

The content of cmd (on entry) is undefined and is ignored.

(BTW, I'm not a C++ expert either.)