illarion / gonotify

Inotify wrapper
MIT License
30 stars 5 forks source link

Integer overflow #9

Closed akemrir closed 1 month ago

akemrir commented 1 month ago

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?

illarion commented 1 month ago

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

illarion commented 1 month ago

@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?

akemrir commented 1 month ago

@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 commented 1 month ago

addressed with: #10