Comment 22 for bug 1891157

Hi Seth,

thank you for security review of the ipp-usb package. My few comments:

> resp is used before a nil check; it'll probably just crash if it hits this

resp is always used after check of returned error, and if this check passed, resp cannot be nil. In one place resp is explicitly checked against nil after error check and a couple of uses. This nil check is excessive and probably present due to historic reasons, I will remove it

> sha1 used for uuid generation

Yes, and in this context cryptographic properties of the SHA1 hash are not important

> gosec identified a lot of cases of ignoring error returns

I reviewed them all, and they all are not critical and falls into the following categories:

1) Error can't happen, or no actions required if it has happened. For example, if I can't set TCP keep-alive parameters (which is very unlikely to happen), OK, I can continue with default parameters. And if I can't send few bytes to the HTTP client, because client has closed its connection, nobody cares about these lost bytes.

2) Error can happen, but reasonable recovery not possible, though it is possible to continue ignoring the error. For example, if I can't write to log file, OK, part of logs will be lost, but I can't report this situation anyway.

3) Further (dependent on failed) actions perform appropriate error checking. For example, I don't check error when creating lock file directory, but attempt to create a lock file in that directory is checked.

4) Error may cause some non-critical functionality to be lost, but program will still continue to work correctly. For example, there is some state associated with each device, if it can't be saved due to file I/O error, it is not considered a serious error. Program can live without it, with some minor regression on its convenience (assigned TCP ports or DNS-SD name override, assigned during DNS-SD name conflict resolution, will not be preserved between sessions, for example)

> Extensive use of atomic operations worries me that locking should have been used instead

Atomics used mostly with purpose to minimize cost of USB connection allocation/deallocation instrumentation and to achieve a near-zero overhead, when debug logging is actually turned off. Even in a case of mistake, logs will be slightly inaccurate, but it will not cause any serious synchronization problems.