Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PVR] Fix time_t db insertion (typecast time_t -> uint) #12069

Merged
merged 1 commit into from
May 10, 2017
Merged

[PVR] Fix time_t db insertion (typecast time_t -> uint) #12069

merged 1 commit into from
May 10, 2017

Conversation

peak3d
Copy link
Contributor

@peak3d peak3d commented May 7, 2017

[PVR] Fix time_t db insertion (typecast time_t -> uint)

Description

Win32's time_t is 64 bit, %u for time_t fails in epg and channel insertion with my win32 build.
This PR typecasts the time_t values used into the type expected in formatting string.

Motivation and Context

Fix crash on EPG update / pvr stall at kodi startup

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

ping @ksooo

Copy link
Member

@Jalle19 Jalle19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a better modifier we could use to avoid casting everywhere?

@ksooo
Copy link
Member

ksooo commented May 8, 2017

Wondering why not every win32 build user experiences those crashes/stalls? Some of the affected functions should be called unconditionally as soon as pvr is enabled.

@peak3d
Copy link
Contributor Author

peak3d commented May 8, 2017

AFAIK there is no format specifier for time_t.
If you push time_t on the list of va_args it has to meet the sizes of the format string.

But yes, @ksooo I was also surprised about the heavy crashes I never realized before.
But, IIRC I never tried PVR on my win machine before.

Behavier is currently (empty dp):

  • kodi starts well and db is filled.
  • after one minute epg want to update -> crash (access on invalid string when formattig)
  • after kodi restart: INSERT CHANNEL segfaults every time with the same issue.

Could be that I use a newer WinSDK than on jenkins build machine and time_t has changed.

@Jalle19 what is your objection regarding type-casts?
@ksooo the EPG functions were NOT called on pvr enable / kodi start time.

time is a wrapper for _time64 and time_t is, by default, equivalent to __time64_t. If you need to force the compiler to interpret time_t as the old 32-bit time_t, you can define _USE_32BIT_TIME_T. This is not recommended because your application may fail after January 18, 2038; the use of this macro is not allowed on 64-bit platforms.

@peak3d
Copy link
Contributor Author

peak3d commented May 8, 2017

Just to avoid a long discussion:

  • We all agree that time_t can be 32 or 64 bit, do we?
  • We all agree that %u is wrong for 64 bit data types, doesn't we?

So it has to be changed in any way, solution one the typecast as done in the commit, solution 2: change the datatypes / return values of the function to unsigned int instead time_t., solution 3: use std::stringstream for formatting.

We only have to decide to one of those 3.

@Rechi
Copy link
Member

Rechi commented May 8, 2017

time_t was 32 bit before #11739 was merged.

@peak3d
Copy link
Contributor Author

peak3d commented May 8, 2017

thanx for sorting this out @Rechi

Solution 4:) Introduce an kodi_time_t Type.

@Paxxi
Copy link
Member

Paxxi commented May 8, 2017

I think we should avoid casting to 32bit,won't be an issue until 2038 but it would feel cleaner to cast to uint64_t everywhere

@Rechi
Copy link
Member

Rechi commented May 8, 2017

I also think there is a better solution then casting.
@Paxxi it gets casted to unsigned int so the problematic year will be 2106.

@xhaggi
Copy link
Member

xhaggi commented May 8, 2017

We have a platform specific define (PRIu64) for unsigned long long (64bit) formatting.
Cast time_t to unsigned long long and use %" PRIu64".

@xhaggi
Copy link
Member

xhaggi commented May 8, 2017

BTW we should think about to drop the time_t here and use our datetime class and datetime sql type as we do it in our video db.

@ksooo
Copy link
Member

ksooo commented May 8, 2017

BTW we should think about to drop the time_t here and use our datetime class and datetime sql type as we do it in our video db.

+1

@Paxxi
Copy link
Member

Paxxi commented May 8, 2017

@Rechi I thought a time_t was unsigned but yeah in that case I don't have any objections really.

@Rechi Rechi added Type: Fix non-breaking change which fixes an issue Component: PVR v18 Leia Platform: Windows labels May 8, 2017
@peak3d
Copy link
Contributor Author

peak3d commented May 10, 2017

Can we pls. agree to one of both:

1.) leave the PR as it is (90 years safe)
2.)

Cast time_t to unsigned long long and use %" PRIu64"

(Safe until end of everything but needs a new db schema)

Does anyone of us believe that we'll have live-tv in 90 years?

@Rechi
Copy link
Member

Rechi commented May 10, 2017

I'm fine with both although version 2 (no casting) is my preferred one.

@peak3d
Copy link
Contributor Author

peak3d commented May 10, 2017

version 2 needs casting in the other direction.
because on linux 32 we still have 32-bit time_t

@Rechi
Copy link
Member

Rechi commented May 10, 2017

Just seen it, pick the version you prefer.

@ksooo
Copy link
Member

ksooo commented May 10, 2017

I'm fine with option 1, merge the PR as it is now.

@fritsch
Copy link
Member

fritsch commented May 10, 2017

For history:

Dear people that come to this world after us. If you find that commit and it bites you in the ass, sit down, drink a beer and be happy that this part of the code survived that long.

@ksooo
Copy link
Member

ksooo commented May 10, 2017

jenkins build this please

@MartijnKaijser MartijnKaijser merged commit 0da9b37 into xbmc:master May 10, 2017
@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone May 10, 2017
@peak3d peak3d deleted the time_t branch May 14, 2017 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Platform: Windows Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants