dominikbraun / timetrace

A simple CLI for tracking your working time.
Apache License 2.0
679 stars 75 forks source link

`timetrace create record` panics because of nil pointer dereference #145

Closed dominikbraun closed 3 years ago

dominikbraun commented 3 years ago

There seems to be a bug in the timetrace create record command:

dominik.braun@Chefkoch3027 % timetrace create record web-store today 14:00 16:07
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12c8acd]

goroutine 1 [running]:
github.com/dominikbraun/timetrace/core.collides(0x0, 0xed86d0240, 0x15f0860, 0xc0001363a8, 0xc00010d8d0, 0x0, 0xc00010dab0, 0x2, 0x2, 0x0)
    /home/circleci/project/core/timetrace.go:306 +0x14d
github.com/dominikbraun/timetrace/core.(*Timetrace).RecordCollides(0xc00014a5e0, 0x0, 0xed86d0240, 0x15f0860, 0xc0001363a8, 0xc00010d8d0, 0x0, 0x0, 0xed86d2004, 0x15f0860)
    /home/circleci/project/core/timetrace.go:295 +0x15d
github.com/dominikbraun/timetrace/cli.createRecordCommand.func1(0xc0001aca00, 0xc000128a80, 0x4, 0x4)
    /home/circleci/project/cli/create.go:95 +0x3fc
github.com/spf13/cobra.(*Command).execute(0xc0001aca00, 0xc000128a00, 0x4, 0x4, 0xc0001aca00, 0xc000128a00)
    /go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:856 +0x2c2
github.com/spf13/cobra.(*Command).ExecuteC(0xc0001ac280, 0x14061b8, 0x7, 0xc0001ac280)
    /go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:960 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
    /go/pkg/mod/github.com/spf13/cobra@v1.1.3/command.go:897
main.main()
    /home/circleci/project/main.go:24 +0x28a

The error occurs in timetrace.go:306, which is the collides function checking a value that is nil.

https://github.com/dominikbraun/timetrace/blob/0c9984c41fcf1e50ac4eb467dc4c0be993bbd44a/core/timetrace.go#L298-L312

I suspect that rec.End is nil here because my latest record hasn't been stopped yet:

dominik.braun@Chefkoch3027 % timetrace list records today
+-----+------------------+-----------+---------+-----------+------------+
|  #  |       KEY        |  PROJECT  |  START  |    END    |  BILLABLE  |
+-----+------------------+-----------+---------+-----------+------------+
|   1 | 2021-06-29-16-08 | web-store | 16:08   | ---       | no         |
|   2 | 2021-06-29-09-05 | web-store | 09:05   | 13:02     | no         |
+-----+------------------+-----------+---------+-----------+------------+
|                                                TOTAL:    | 3H 57MIN   |
+-----+------------------+-----------+---------+-----------+------------+

Checking for those values to be non-nil shouldn't be too hard.

alksmt commented 3 years ago

Is it supposed that a record could be created in future? If so, we are having a second issue: when a record in future is created, and another record is on track now, it would potentially collide with each other and we will end up with overlaping records.

As a solution, records in future could be rejected or rejected only if another record is still on track, but with the second way potential overlapping is still possible.

Probably issue with records in future is not so related to this one and should be created separately.

dominikbraun commented 3 years ago

Is it supposed that a record could be created in future? If so, we are having a second issue: when a record in future is created, and another record is on track now, it would potentially collide with each other and we will end up with overlaping records.

As a solution, records in future could be rejected or rejected only if another record is still on track, but with the second way potential overlapping is still possible.

Probably issue with records in future is not so related to this one and should be created separately.

I think it would make sense to reject records in the future so that records can only be created retroactively. You may open an issue for this topic if you want to, otherwise I can do it 😄

alksmt commented 3 years ago

Will create one 👍

alksmt commented 3 years ago

Created PR for this bug, with some simpification for collides check, going to also handle future records rejection in new issue.