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

goQuery: 18.45 E is shown in drops column of list target on freshly converted DB #154

Closed
2 tasks done
els0r opened this issue Aug 2, 2023 · 7 comments · Fixed by #163
Closed
2 tasks done

goQuery: 18.45 E is shown in drops column of list target on freshly converted DB #154

els0r opened this issue Aug 2, 2023 · 7 comments · Fixed by #163
Assignees
Labels
bug Something isn't working
Milestone

Comments

@els0r
Copy link
Owner

els0r commented Aug 2, 2023

Just saw this with a large DB that was recently converted (spanning 180 days and 0.66 PB of captured traffic):

       eth7     11.34 M      9.31 M      20.38 GB      1.77 GB      248.41 k        0.00       18.45 E    2023-02-02 00:55:24    2023-08-01 22:23:50

Notice the 18.45 E. I have a feeling like this is an overflow and actually means 0. If you round 2^64 = 1.844674407370955e19 and apply humanization, you get 18.45 E.

DoD:

  • find out if this is part of the conversion logic (legacy)
  • or part of the list target (goQuery)

If it is the former, it needs to be in the v4 milestone. If not, we can fix this minor bug later.

@els0r els0r added the bug Something isn't working label Aug 2, 2023
@els0r els0r added this to the v4 Release milestone Aug 2, 2023
@fako1024
Copy link
Collaborator

fako1024 commented Aug 3, 2023

@els0r Assuming that you can't hand over that interface's data for me to analyze you should easily be able to find out if it's from the conversion by looking at the code in legacy main.go here:

		bulkWorkload = append(bulkWorkload, goDB.BulkWorkload{
			FlowMap: block.data,
			CaptureMeta: goDB.CaptureMetadata{
				PacketsDropped: blockMetadata.PcapPacketsDropped + blockMetadata.PcapPacketsIfDropped,
			},
			Timestamp: block.ts,
		})

Afaik this is the only place where drops are transferred / written to the converted DB, so by logging / analyzing what's actually written it should be easy to discern if there's some overflow and / or issue with the math (or, simply, corrupted input). Let me know if I can be of any assistance...

@els0r
Copy link
Owner Author

els0r commented Aug 3, 2023

I'm going to bet a Lunch IPA on this being the culprit. Mainly because it confirms my belief about an overflow 😀 . Taken from ./1676937600/meta.json of an interface having 18.45 E drops

    {
      "timestamp": 1676972734,
      "pcap_packets_received": -1,
      "pcap_packets_dropped": -1,
      "pcap_packets_if_dropped": -1,
      "packets_logged": 0,
      "flowcount": 0,
      "traffic": 0
    }

In a ring buffer, if you go back -1 from 0, you'll end up at the highest possible position of that buffer, aka 2^64 - 1 for a uint64 or 0xFFFFFFFFFFFFFFFF. Or it's another phenomenon.

Look closer. What we have in the PacketsDropped is an int, which defaults to int64 in a 64-bit architecture. -1 of an int64 will be represented as - you guessed it 0xFFFFFFFFFFFFFFFF. If you convert that sequence of bits back to a uint64, you'll get the highest possible value for a uint64.

Who knew -1 could be so big 😏 .

The only thing I haven't been able to reproduce is the sequence of events. What I suspect is, that through the use of the binary package in the GPDir Marshal/Unmarshal, we manage to evade the type safety of uint64. And it's not an obvious one, since if you do uint64(-1), the compiler will complain.

If we don't want to go down this rabbit hole, we could simply default back to 0 if a -1 is encountered in the metadata and check if the bug disappears. But for educational purposes, I'd like to get your take on this. Maybe you spot the obvious.

@fako1024
Copy link
Collaborator

fako1024 commented Aug 4, 2023

Not gonna bet against it (but I'll take the IPA anyway), had I known that the value could be negative I'd added more safety here. Thing is, yes, the compiler will guard against obvious stupidity such as trying uint64(-1), but it doesn't know the value of an int variable (at compile time) that you are casting to uint64. Easily reproducible with e.g. this (which is almost as dumb and exactly what happens during Marshal()): https://go.dev/play/p/XEo2mcC8N80

var testVal int
testVal = -1

fmt.Println(uint64(testVal)) // <- Happily compiles and prints the aforementioned value. 

I'll add a fix in a PR (forcing -1 to 0).

@fako1024
Copy link
Collaborator

fako1024 commented Aug 4, 2023

Rasied fako1024/slimcap#52 for corresponding changes in slimcap. @els0r Please approve linked PR asap so I can bump the version here.

@els0r
Copy link
Owner Author

els0r commented Aug 4, 2023

Rasied fako1024/slimcap#52 for corresponding changes in slimcap. @els0r Please approve linked PR asap so I can bump the version here.

Done

@els0r
Copy link
Owner Author

els0r commented Aug 4, 2023

Not gonna bet against it (but I'll take the IPA anyway), had I known that the value could be negative I'd added more safety here. Thing is, yes, the compiler will guard against obvious stupidity such as trying uint64(-1), but it doesn't know the value of an int variable (at compile time) that you are casting to uint64. Easily reproducible with e.g. this (which is almost as dumb and exactly what happens during Marshal()): https://go.dev/play/p/XEo2mcC8N80

var testVal int
testVal = -1

fmt.Println(uint64(testVal)) // <- Happily compiles and prints the aforementioned value. 

I'll add a fix in a PR (forcing -1 to 0).

HA! Nice one. Of course the compiler complains if it is a constant because it knows the value on compile time 💡 . Well, at least we didn't blow up a rocket 🤷 .

@fako1024
Copy link
Collaborator

fako1024 commented Aug 4, 2023

HA! Nice one. Of course the compiler complains if it is a constant because it knows the value on compile time . Well, at least we didn't blow up a rocket 🤷 .

Or (temporarily) "lose" (i.e. kill) contact with a deep space probe after sending the wrong command(s): https://www.businessinsider.com/nasa-loses-contact-voyager-2-sent-wrong-command-mistake-space-2023-8 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants