Opened 3 years ago

Closed 3 years ago

#3673 closed defect (fixed)

pgsql unit tests failing

Reported by: wlodekwencel Owned by: marcin
Priority: medium Milestone: Kea0.9.1
Component: database-backend Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

There are two unit tests that are failing (ubuntu 14.04): PgSqlLeaseMgrTest?.maxDate4 and PgSqlLeaseMgrTest?.maxDate6

[ RUN      ] PgSqlLeaseMgrTest.maxDate4
generic_lease_mgr_unittest.cc:697: Failure
Expected: lmptr_->addLease(leases[1]) throws an exception of type DbOperationError.
  Actual: it throws nothing.
NOTICE:  there is no transaction in progress
[ RUN      ] PgSqlLeaseMgrTest.maxDate6
generic_lease_mgr_unittest.cc:866: Failure
Expected: lmptr_->addLease(leases[1]) throws an exception of type DbOperationError.
  Actual: it throws nothing.
NOTICE:  there is no transaction in progress

Link to errors on Jenkins project:
ERROR1
ERROR2

Subtickets

Change History (10)

comment:1 Changed 3 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9.1
  • Priority changed from medium to low

comment:2 Changed 3 years ago by tomek

  • Milestone changed from Kea0.9.1beta to Kea0.9.1

Moving from 0.9.1beta to 0.9.1.

comment:3 Changed 3 years ago by marcin

  • Priority changed from low to medium

comment:4 Changed 3 years ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:5 Changed 3 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

The overflows during the time conversions were not handled properly. On 32-bit systems the time_t is typically implemented as the int32_t value in which case adding anything to the !LeaseMgr::MAX_DB_TIME resulted in overflow.

I also fixed some trivial issues reported by the cppcheck for both backends.

Proposed ChangeLog entry:

XXX.	[bug]		marcin
	Handle overflows during time conversions in the MySQL and
	PostgreSQL lease database backends.
	(Trac #3673, git abcd)

comment:6 Changed 3 years ago by sar

  • Owner changed from UnAssigned to sar

comment:7 follow-up: Changed 3 years ago by sar

  • Owner changed from sar to marcin

Fixed one typo

src/lib/dhcpsrv/pgsql_lease_mgr.cc

There is a comment staring at line 318 that PostgresSQL does funny things with time past Y2038 and that they are disallowed. But I don't see any checks for > 2038. Should the comment be updated? (As a side note as I recall we have had people trying to use leases
that were long enough to exceed that time - if we do include such a check we should mention it in the notes somewhere.)

Other items

I haven't walked through all of the code yet, but should the memfile code also be getting tested and should it be rejecting overly high values? It seems to do the same addition as the *SQL databases were doing and I'm not sure about the conversions in effect.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by marcin

  • Owner changed from marcin to sar

Replying to sar:

Fixed one typo

Thanks.

src/lib/dhcpsrv/pgsql_lease_mgr.cc

There is a comment staring at line 318 that PostgresSQL does funny things with time past Y2038 and that they are disallowed. But I don't see any checks for > 2038. Should the comment be updated? (As a side note as I recall we have had people trying to use leases
that were long enough to exceed that time - if we do include such a check we should mention it in the notes somewhere.)

This comment had been there previously and I didn't change it. But, I updated it now.

Other items

I haven't walked through all of the code yet, but should the memfile code also be getting tested and should it be rejecting overly high values? It seems to do the same addition as the *SQL databases were doing and I'm not sure about the conversions in effect.

With the Memfile there is a different story because it stores the time_t values as it gets them and performs no conversions to anything else. I think the Memfile may be in trouble on 32-bit systems where the time_t is implemented as int32_t. On those systems there may be overflows but it is a different story and not really related to the use of backend but the assignments like this:

time_t expire = cltt + valid_lft;

We should probably check all places where the timestamps are calculated and the result is stored in the time_t on 32-bit OSes and make sure there are no overflows. But, maybe it is too much burden to support 32-bit systems.

Last edited 3 years ago by sar (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 3 years ago by sar

  • Owner changed from sar to marcin

Replying to marcin:

Replying to sar:

src/lib/dhcpsrv/pgsql_lease_mgr.cc

There is a comment staring at line 318 that PostgresSQL does funny things with time past Y2038 and that they are disallowed. But I don't see any checks for > 2038. Should the comment be updated? (As a side note as I recall we have had people trying to use leases
that were long enough to exceed that time - if we do include such a check we should mention it in the notes somewhere.)

This comment had been there previously and I didn't change it. But, I updated it now.

The new comment is better, thank you.

Other items

I haven't walked through all of the code yet, but should the memfile code also be getting tested and should it be rejecting overly high values? It seems to do the same addition as the *SQL databases were doing and I'm not sure about the conversions in effect.

With the Memfile there is a different story because it stores the time_t values as it gets them and performs no conversions to anything else. I think the Memfile may be in trouble on 32-bit systems where the time_t is implemented as int32_t. On those systems there may be overflows but it is a different story and not really related to the use of backend but the assignments like this:

time_t expire = cltt + valid_lft;

We should probably check all places where the timestamps are calculated and the result is stored in the time_t on 32-bit OSes and make sure there are no overflows. But, maybe it is too much burden to support 32-bit systems.

I've opened 3758 for checking time_t on 32 bit OSes. I'm not sure how much effort we want to devote to such things but we now have a ticket to track it.

You can merge this patch into the master.

comment:10 Changed 3 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks for the review. Merged with commit 27b4e4590fdee507f0e877d7b771dc6c6457a4b and closing the ticket.

Note: See TracTickets for help on using tickets.