Dear VDR folks,
Cppcheck 1.49 [1] reports the following error when run against VDR 1.7.18.
[timers.c:53]: (error) snprintf size is out of bounds
Looking at `timers.c` in `CTimer` `*file = 0` and afterward written to if I am not mistaken.
[…] cTimer::cTimer(bool Instant, bool Pause, cChannel *Channel) { startTime = stopTime = 0; lastSetEvent = 0; recording = pending = inVpsMargin = false; flags = tfNone; if (Instant) SetFlags(tfActive | tfInstant); channel = Channel ? Channel : Channels.GetByNumber(cDevice::CurrentChannel()); time_t t = time(NULL); struct tm tm_r; struct tm *now = localtime_r(&t, &tm_r); day = SetTime(t, 0); weekdays = 0; start = now->tm_hour * 100 + now->tm_min; stop = now->tm_hour * 60 + now->tm_min + Setup.InstantRecordTime; stop = (stop / 60) * 100 + (stop % 60); if (stop >= 2400) stop -= 2400; priority = Pause ? Setup.PausePriority : Setup.DefaultPriority; lifetime = Pause ? Setup.PauseLifetime : Setup.DefaultLifetime; *file = 0; aux = NULL; event = NULL; if (Instant && channel) snprintf(file, sizeof(file), "%s%s", Setup.MarkInstantRecord ? "@" : "", *Setup.NameInstantRecord ? Setup.NameInstantRecord : channel->Name()); […]
Unfortunately I do not know C++ well enough to judge this error message.
Thanks,
Paul
On 15.06.2011 15:30, Paul Menzel wrote:
Dear VDR folks,
Cppcheck 1.49 [1] reports the following error when run against VDR 1.7.18.
[timers.c:53]: (error) snprintf size is out of bounds
Looking at `timers.c` in `CTimer` `*file = 0` and afterward written to if I am not mistaken.
This just sets the string to be "empty", but...
[…] cTimer::cTimer(bool Instant, bool Pause, cChannel *Channel) { ... *file = 0; aux = NULL; event = NULL; if (Instant&& channel) snprintf(file, sizeof(file), "%s%s", Setup.MarkInstantRecord ? "@" : "", *Setup.NameInstantRecord ? Setup.NameInstantRecord : channel->Name());
...this should be
sizeof(file) - 1
Thanks for the bug report.
Klaus
[…]Unfortunately I do not know C++ well enough to judge this error message.
Thanks,
Paul
Am 15.06.2011 18:34, schrieb Klaus Schmidinger:
On 15.06.2011 15:30, Paul Menzel wrote:
if (Instant&& channel) snprintf(file, sizeof(file), "%s%s",Setup.MarkInstantRecord ? "@" : "", *Setup.NameInstantRecord ? Setup.NameInstantRecord : channel->Name());
...this should be
sizeof(file) - 1
Actually, all versions of snprintf documentation I've just checked agree that snprintf will write at most size-1 chars and a trailing 0 byte, so it was ok before too. But for safety, on byte less doesn't hurt.
Or is there some broken implementation out there that may write beyond str[size-1]?
(strncpy is more broken, thats why my typical usage is: strncpy(dest, src, sizeof(dest)-1); dest[sizeof(dest)-1] = 0; )
Cheers,
Udo
On 15.06.2011 19:37, Udo Richter wrote:
Am 15.06.2011 18:34, schrieb Klaus Schmidinger:
On 15.06.2011 15:30, Paul Menzel wrote:
if (Instant&& channel) snprintf(file, sizeof(file), "%s%s",Setup.MarkInstantRecord ? "@" : "", *Setup.NameInstantRecord ? Setup.NameInstantRecord : channel->Name());
...this should be
sizeof(file) - 1
Actually, all versions of snprintf documentation I've just checked agree that snprintf will write at most size-1 chars and a trailing 0 byte, so it was ok before too. But for safety, on byte less doesn't hurt.
Gee, you're right!
Or is there some broken implementation out there that may write beyond str[size-1]?
None that I know of.
Well, since the docs for snprintf are clear about this, let's leave things as they are.
I wonder, though, why cppcheck thinks there's something wrong here...
Klaus
Am Wed, 15 Jun 2011 18:34:59 +0200 schrieb Klaus Schmidinger Klaus.Schmidinger@tvdr.de:
...this should be
sizeof(file) - 1
Thanks for the bug report.
This is no bug. The size parameter includes the '\0' byte.
Gerald
-
Gerald Dachs -
Klaus Schmidinger -
Paul Menzel -
Udo Richter