gonotify icon indicating copy to clipboard operation
gonotify copied to clipboard

Integer overflow

Open akemrir opened this issue 1 year ago • 3 comments

Hi @illarion , please discuss this one: inotify.go:111:21 gosec G115: integer overflow conversion int -> uint32 inotify.go:200:36 gosec G115: integer overflow conversion int32 -> uint32 inotify.go:208:22 gosec G115: integer overflow conversion int32 -> uint32

I see the inconsequence problem in signatures of syscall. wd, err := syscall.InotifyAddWatch(fd, req.pathName, req.mask) this returns wd int, on 64bit systems int64

And then, this one expects uint32 _, err := syscall.InotifyRmWatch(fd, wd)

Internally uint32 were used propably because of other signatures.

int64 was taken into consideration? Or int in general to adjust to operating system?

On the other hand. How high values are passed there? No one would watch 4.294.967.295 of watches.

I saw that this number grows one by one when adding watches, so it's max is as of number of watches. Or ocasionally process would need to reinit, causing watches number to reset.

What's your opinion on this?

akemrir avatar Sep 16 '24 19:09 akemrir

I am in progress of fixing concurrency issues and when I'll finish it will come back to this issue

illarion avatar Sep 17 '24 09:09 illarion

@akemrir Well, since this syscalls have such a discrepancy, what can we do? Even if we convert everything into int or int64, the moment gonotify uses syscall.InotifyRmWatch() we are forced to convert it down to uint32.

Do you have a proposal how it could be fixed?

illarion avatar Sep 18 '24 08:09 illarion

@illarion What if we could:

	const MaxUint32asInt = int(^uint32(0))
        
        //from InotifyAddWatch
	var wd int = 5_294_967_295
	var localWd uint32

	if wd > MaxUint32asInt {
		// new Error here
		return log.Fatal("Watch counter overflow error")
	}
	localWd = uint32(wd)

I think this way: it would be better and safer to control possible error. To have controlled situation, rather than result in panic. Even though they possibly wont get there, for very long time running process it may happen.

Then outside developer can handle error, log it to logs and reload app internally.

math package have math.MaxUint32 as int

akemrir avatar Sep 20 '24 05:09 akemrir

addressed with: #10

akemrir avatar Oct 09 '24 17:10 akemrir