facebook/time

Meta's Time libraries

time's Issues

test failures on 32bit architectures

When building 160580e for i686 or armv7hl:

github.com/facebookincubator/ntp/protocol/ntpshm
--- FAIL: Test_NTPSHMStruct (0.00s)
    ntpshm_test.go:45: 
        	Error Trace:	ntpshm_test.go:45
        	Error:      	Not equal: 
        	            	expected: 96
        	            	actual  : 88
        	Test:       	Test_NTPSHMStruct
    ntpshm_test.go:48: 
        	Error Trace:	ntpshm_test.go:48
        	Error:      	Not equal: 
        	            	expected: ntpshm.NTPSHM{Mode:1, Count:18668, ClockTimeStampSec:1615562094, ClockTimeStampUSec:892026, ReceiveTimeStampSec:1615562056, ReceiveTimeStampUSec:457385, Leap:0, Precision:-30, Nsamples:0, Valid:0, ClockTimeStampNSec:892026694, ReceiveTimeStampNSec:457385010, Dummy:[8]int32{0, 0, 0, 0, 0, 0, 0, 0}}
        	            	actual  : ntpshm.NTPSHM{Mode:1, Count:18668, ClockTimeStampSec:1615562094, ClockTimeStampUSec:892026, ReceiveTimeStampSec:6938786195178520576, ReceiveTimeStampUSec:0, Leap:457385, Precision:0, Nsamples:-30, Valid:0, ClockTimeStampNSec:0, ReceiveTimeStampNSec:892026694, Dummy:[8]int32{457385010, 0, 0, 0, 0, 0, 0, 0}}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -5,12 +5,12 @@
        	            	  ClockTimeStampUSec: (int32) 892026,
        	            	- ReceiveTimeStampSec: (int64) 1615562056,
        	            	- ReceiveTimeStampUSec: (int32) 457385,
        	            	- Leap: (int32) 0,
        	            	- Precision: (int32) -30,
        	            	- Nsamples: (int32) 0,
        	            	+ ReceiveTimeStampSec: (int64) 6938786195178520576,
        	            	+ ReceiveTimeStampUSec: (int32) 0,
        	            	+ Leap: (int32) 457385,
        	            	+ Precision: (int32) 0,
        	            	+ Nsamples: (int32) -30,
        	            	  Valid: (int32) 0,
        	            	- ClockTimeStampNSec: (int32) 892026694,
        	            	- ReceiveTimeStampNSec: (int32) 457385010,
        	            	+ ClockTimeStampNSec: (int32) 0,
        	            	+ ReceiveTimeStampNSec: (int32) 892026694,
        	            	  Dummy: ([8]int32) (len=8) {
        	            	-  (int32) 0,
        	            	+  (int32) 457385010,
        	            	   (int32) 0,
        	Test:       	Test_NTPSHMStruct
    ntpshm_test.go:50: 
        	Error Trace:	ntpshm_test.go:50
        	Error:      	Should be true
        	Test:       	Test_NTPSHMStruct
    ntpshm_test.go:51: 
        	Error Trace:	ntpshm_test.go:51
        	Error:      	Should be true
        	Test:       	Test_NTPSHMStruct
FAIL
exit status 1
FAIL	github.com/facebookincubator/ntp/protocol/ntpshm	0.007s

You can find the full output at https://koji.fedoraproject.org/koji/taskinfo?taskID=69320828 and https://koji.fedoraproject.org/koji/taskinfo?taskID=69320830 (look at build.log). This failure did not happen on an older commit (81cb02c).

Chrony missing IPADDR_UNSPEC

Chrony's ipAddr.Family has an "unspecified" mode for reference clocks. This is used by the tracking packet results in the Chrony C code to figure out if it's a reference clock and print the name using the RefidToString() function.

If we don't reproduce format_name() from client.c, we end up with bugs like SuperQ/chrony_exporter#7.

feature: Add String() to chrony ModeType

It would be nice to have a String() helper method on the Chrony ModeType, similar to SourceStateType.

OSS-Fuzz issue 64287

OSS-Fuzz has found a bug in this project. Please see https://oss-fuzz.com/testcase?key=5939434687954944 for details and reproducers.

This issue is mirrored from https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64287 and will auto-close if the status changes there.

If you have trouble accessing this report, please file an issue at https://github.com/google/oss-fuzz/issues/new.

OSS-Fuzz issue 64308

OSS-Fuzz has found a bug in this project. Please see https://oss-fuzz.com/testcase?key=5052870759940096 for details and reproducers.

This issue is mirrored from https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64308 and will auto-close if the status changes there.

If you have trouble accessing this report, please file an issue at https://github.com/google/oss-fuzz/issues/new.

FB.me

Compiling for AIX results in "undefined: socketControlMessageTimestamp"

I'm trying to build ntpcheck for an AIX 7.2 system. The following build command should work.

GOOS=aix GOARCH=ppc64 go build -trimpath -ldflags="-s -w" -o ntpcheck-aix ./cmd/ntpcheck

Instead, I get the following:

go: downloading github.com/fatih/color v1.13.0
go: downloading github.com/sirupsen/logrus v1.8.1
go: downloading github.com/spf13/cobra v1.4.0
go: downloading golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a
go: downloading github.com/pkg/errors v0.9.1
go: downloading github.com/mattn/go-colorable v0.1.12
go: downloading github.com/mattn/go-isatty v0.0.14
go: downloading github.com/spf13/pflag v1.0.5
# github.com/facebook/time/timestamp
timestamp/timestamp.go:105:20: undefined: socketControlMessageTimestamp

It is not immediately obvious to me how to fix this but I suspect it's an endianness issue.

time/ntp/chrony: The 'falseticker' source state is called 'falseticket'

time/ntp/chrony/packet.go uses the name 'falseticket' for the Chrony source state of 'falseticker' (this is the state RPY_SD_ST_FALSETICKER in https://github.com/mlichvar/chrony/blob/master/candm.h, the source of the constants in packet.go according to a comment there). This mis-naming is in both the name of the constant, SourceStateFalseTicket, as well as the string version in SourceStateDesc. Since this is an exposed API constant, I think the best that can be done at this point without a breaking change is create a new name for the same constant value, SourceStateFalseTicker, and change the string version in SourceStateDesc (which hopefully wouldn't be considered a breaking API change).

Add unittests to chrony packet parsing

Add tests to packet.go, along with instructions on how to capture the raw data needed for it.

Calnex firmware update doesn't work with final release tarballs

The Calnex firmware file string is slightly different for final releases.

The previously released versions of firmware have been engineering releases.
sentinel_fw_v2.17.0.0.6988-20220406.tar

Final releases have the structure.
sentinel_fw_R17.0.0.tar

Sending these files gets the response
time="2022-11-21T14:36:41Z" level=fatal msg="Malformed version: sentinel_fw_r17.0.0"

PTP experiences

not an issue, but a comment to this work:

Its great to see global distributed companies improving their NTP timekeeping!

The Finantial sector acheived even better accuracy years ago by upgrading to PTP, please see here:
https://github.com/pestrela/papers/blob/master/3_Clock_Sync/2014%20-%20Estrela%20ISPCS14%20-%20Best%20Paper%20-%20Increasing%20PTPv2%20robustness%20-%20presentation.pdf

The scientific papers describe everything that was necessary to do put a standard to work "in the real world":
https://github.com/pestrela/papers/tree/master/3_Clock_Sync

Adjust logging in library packges

Using the chrony library forces users into using the logrus logging library. It's not a terrible choice, but it makes it such that you're stuck with it, even if you have other logging.

It would be nice if you could register a logger, similar to how we do this in gosnmp.

[oss-fuzz] upstream support request

@abulimov, do you support the integration of time with google/oss-fuzz#11095 to help find bugs and crashes like #301 through automated fuzzing? We would need to add the email addresses of the maintainers who will receive potential bug reports to a public list in the oss-fuzz repository

Why there isn't support for udp4?

I am trying to make a RunCheck with a provided ipv4:123 and I got the error dial udp6: address X.X.X.X: no suitable address found

Will be implemented?

OSS-Fuzz issue 64279

OSS-Fuzz has found a bug in this project. Please see https://oss-fuzz.com/testcase?key=4666103720443904 for details and reproducers.

This issue is mirrored from https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64279 and will auto-close if the status changes there.

If you have trouble accessing this report, please file an issue at https://github.com/google/oss-fuzz/issues/new.

feature: add hardware RX timestamp support

Right now, this code is using SO_TIMESTAMPNS/SO_TIMESTAMP, which use kernel level RX timestamps. The documentation and code comment state "hardware timestamps", which is misleading. These timestamps come from https://elixir.bootlin.com/linux/latest/C/ident/net_timestamp_check while "hardware timestamps" would come from 1588 timer hardware in the NIC itself.

https://www.kernel.org/doc/Documentation/networking/timestamping.txt has documentation on the SO_TIMESTAMPING flag, which accepts SOF_TIMESTAMPING_RX_HARDWARE. NIC 1588 hardware timers are usually not synchronized to system time, so your software would need to do that as well. chrony has an example of all of this.

This is just the RX timestamps. TX timestamps could be enabled in NTP interleaved mode, but that would mean keeping state per client. I'm assuming keeping state per client would conflict with the other goals of this project.

add unit tests for reboot.go

I would like to add tests for calnex/cmd/reboot.go

Fedora 37 dependency issue

We have fedora 37 build issues:

Error: 
 Problem: package golang-google-grpc-devel-1.48.0-7.fc37.noarch requires golang(contrib.go.opencensus.io/exporter/stackdriver), but none of the providers can be installed
  - package golang-google-grpc-devel-1.48.0-7.fc37.noarch requires golang(contrib.go.opencensus.io/exporter/stackdriver/monitoredresource), but none of the providers can be installed
  - package golang-github-kit-devel-0.11.0-4.fc37~bootstrap.noarch requires golang(google.golang.org/grpc), but none of the providers can be installed
  - package golang-github-kit-devel-0.11.0-4.fc37~bootstrap.noarch requires golang(google.golang.org/grpc/codes), but none of the providers can be installed
  - package golang-github-kit-devel-0.11.0-4.fc37~bootstrap.noarch requires golang(google.golang.org/grpc/status), but none of the providers can be installed
  - package golang-github-kit-devel-0.11.0-4.fc37~bootstrap.noarch requires golang(google.golang.org/grpc/metadata), but none of the providers can be installed
  - package golang-contrib-opencensus-exporter-stackdriver-devel-0.13.13-1.fc37.noarch requires golang(github.com/prometheus/prometheus/model/value), but none of the providers can be installed
  - package golang-x-exp-devel-0-0.45.20220330git053ad81.fc37.noarch requires golang(github.com/go-kit/kit/log), but none of the providers can be installed
  - package golang-github-prometheus-devel-2.32.1-6.fc37.noarch requires golang(github.com/docker/docker/api/types), but none of the providers can be installed
  - package golang-github-prometheus-devel-2.32.1-6.fc37.noarch requires golang(github.com/docker/docker/client), but none of the providers can be installed
  - package golang-github-prometheus-devel-2.32.1-6.fc37.noarch requires golang(github.com/docker/docker/api/types/filters), but none of the providers can be installed
  - package golang-github-prometheus-devel-2.32.1-6.fc37.noarch requires golang(github.com/docker/docker/api/types/swarm), but none of the providers can be installed
  - conflicting requests
  - nothing provides golang(github.com/containerd/cgroups/stats/v1) needed by golang-github-docker-devel-22.06.0~beta.0-4.fc37.noarch
  - nothing provides golang(github.com/containerd/cgroups/v2) needed by golang-github-docker-devel-22.06.0~beta.0-4.fc37.noarch
  - nothing provides golang(github.com/containerd/cgroups/v2/stats) needed by golang-github-docker-devel-22.06.0~beta.0-4.fc37.noarch
(try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)

Should we consider disabling it in packit config?

OSS-Fuzz issue 64288

OSS-Fuzz has found a bug in this project. Please see https://oss-fuzz.com/testcase?key=6494096629563392 for details and reproducers.

This issue is mirrored from https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64288 and will auto-close if the status changes there.

If you have trouble accessing this report, please file an issue at https://github.com/google/oss-fuzz/issues/new.

Panic in `prepareExpression`

Bug Note:

file: fbclock/daemon/math_test.go, ptp/c4u/clock/math_test.go

func TestPrepareExpression_bug(t *testing.T) {

	prepareExpression(string("\\"))
}

run : go test -run TestPrepareExpression_bug $(pwd)/fbclock/daemon

backtrace

--- FAIL: TestPrepareExpression_bug (0.00s)
panic: runtime error: index out of range [1] with length 1 [recovered]
	panic: runtime error: index out of range [1] with length 1

goroutine 79 [running]:
testing.tRunner.func1.2({0x9a8900, 0xc000296660})
	/usr/lib/golang/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/usr/lib/golang/src/testing/testing.go:1529 +0x39f
panic({0x9a8900, 0xc000296660})
	/usr/lib/golang/src/runtime/panic.go:884 +0x213
github.com/Knetic/govaluate.(*lexerStream).readCharacter(...)
	/home/0x34d/tools/go/pkg/mod/github.com/!knetic/[email protected]+incompatible/lexerStream.go:28
github.com/Knetic/govaluate.readUntilFalse(0xc000085d60, 0x0, 0x1, 0x1, 0xa044b0)
	/home/0x34d/tools/go/pkg/mod/github.com/!knetic/[email protected]+incompatible/parsing.go:272 +0x473
github.com/Knetic/govaluate.readTokenUntilFalse(0xc00000005c?, 0xc0000287a8?)
	/home/0x34d/tools/go/pkg/mod/github.com/!knetic/[email protected]+incompatible/parsing.go:249 +0x4c
github.com/Knetic/govaluate.readToken(0xc000085d60, {0x0, 0x1, 0x0, {0xc74ce0, 0x9, 0x9}}, 0x26?)
	/home/0x34d/tools/go/pkg/mod/github.com/!knetic/[email protected]+incompatible/parsing.go:193 +0x9f7
github.com/Knetic/govaluate.parseTokens({0xa62a90, 0x1}, 0x544873?)
	/home/0x34d/tools/go/pkg/mod/github.com/!knetic/[email protected]+incompatible/parsing.go:27 +0x31f
github.com/Knetic/govaluate.NewEvaluableExpressionWithFunctions({0xa62a90, 0x1}, 0x544840?)
	/home/0x34d/tools/go/pkg/mod/github.com/!knetic/[email protected]+incompatible/EvaluableExpression.go:99 +0x9f
github.com/facebook/time/fbclock/daemon.prepareExpression({0xa62a90?, 0x48ded7?})
	/home/0x34d/project/time/fbclock/daemon/math.go:198 +0x5a
github.com/facebook/time/fbclock/daemon.TestPrepareExpression_bug(0xc0002b0340?)
	/home/0x34d/project/time/fbclock/daemon/math_test.go:141 +0x47
testing.tRunner(0xc0002b11e0, 0xa045f8)
	/usr/lib/golang/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/usr/lib/golang/src/testing/testing.go:1629 +0x3ea
exit status 2
FAIL	github.com/facebook/time/fbclock/daemon	0.010s

Test failures on s390x

See https://koji.fedoraproject.org/koji/taskinfo?taskID=83646116 for the full log

github.com/facebook/time/timestamp
--- FAIL: TestReadPacketWithRXTimestamp (0.00s)
    timestamp_linux_test.go:212: 
        	Error Trace:	timestamp_linux_test.go:212
        	Error:      	Not equal: 
        	            	expected: 164641548
        	            	actual  : 910124969297792771
        	Test:       	TestReadPacketWithRXTimestamp
        	Messages:   	kernel timestamps should be within 10s
--- FAIL: TestReadPacketWithRXTXTimestamp (0.00s)
    timestamp_linux_test.go:247: 
        	Error Trace:	timestamp_linux_test.go:247
        	Error:      	Not equal: 
        	            	expected: 164641548
        	            	actual  : 910124970574170190
        	Test:       	TestReadPacketWithRXTXTimestamp
        	Messages:   	kernel timestamps should be within 10s
FAIL
exit status 1
FAIL	github.com/facebook/time/timestamp	0.117s

Custom domain setting is not supported in ptp4u

It's hardcoded to 0 in
https://github.com/facebook/time/blob/main/ptp/ptp4u/server/subscription.go#L205
Need to patch NewSubscriptionClient to allow setting a custom domain.

bad file descriptor

I've been seeing bad file descriptor error message sometimes when trying to read an incoming packet.

Problem:

• The responder creates a net.UDPConn via net.ListenUDP
• Afterwards, it calls connFd to get a file descriptor which uses the File method of the net.UDPConn.
• The File method creates a duplicate:

File returns a copy of the underlying os.File. It is the caller's responsibility to close f when finished.

• The Close method is never explicitly called and the GC takes care of it
• This leads to the mentioned error message, trying to read from a closed fd

Solution:

There are at least 2 alternatives and I wanted to ask for opinions.

• Avoid creating a duplicate fd to read packets: This implies not using net.ListenUDP, (which doesn't expose the "original" fd) and using a syscall directly to create the listening socket.

• Explicitly close the duplicate file descriptor to avoid the GC closing it for us at an undesired time: This implies removing/modifying the connFd function.

Any other alternatives? Thoughts?