http: return from httpGets2() on unhandled errors

[xypron]

Closes: #879

In case of some errors httpGets2 waits for the next packet. In all other cases of errors it should return NULL to avoid negative values of http->used leading to ending up in an endless loop.

[zdohnal]

Hi Heinrich!

Thank you for the PR!

Were you able to verify whether the patch fixes the issue? It looks to me that we return NULL if bytes is negative either way (line 1124), if we don't retry by calling continue.

To be honest I don't see there a way how http->used can become negative at the moment.

Whenever bytes is negative, we either return or retry until bytes > 0, so it does not get assigned into http->used.

When we copy from socket to array line, we only copy until we hit LF, bufend (which is buffer pointer + http->used) or end of line buffer - buffer length is 2048, line length is around 30k.

The only idea I have is that http->used is negative at the start of function, but it does not seem possible from other functions which set http->used...

And the http struct is allocated by calloc, so http->used is initialized to 0 when allocated, so it is not uninitialized either...

Maybe the fact that cups-browsed is asking for the destination in one thread and deleting it in another has an effect? Both threads have a different http structure, but both are connected via domain socket to local cupsd.

@xypron can you get us error log from CUPS when this error in cups-browsed happens? You can enable it by cupsctl LogLevel=debug2 and the file will be in /var/log/cups? I don't expect much will be seen on this level, but I don't want to underestimate it as well.

[xypron]

@zdohnal Thank you for reviewing.

• Would cupsctl set debugging for the cups-browsed process? Isn't /etc/cups/cups-browsed.conf controlling this?
• Shouldn't I put another debug statement into the while (lineptr < lineend) { loop writing the value ofhttp->used?
• Can the debug level be raised while cusp-browsed is running or does it have to be switched on before starting the service?

[zdohnal]

@zdohnal Thank you for reviewing.

* Would cupsctl set debugging for the cups-browsed process? Isn't /etc/cups/cups-browsed.conf controlling this?

cupsctl can set several directives in cupsd.conf, which is a configuration file for cupsd daemon (CUPS) - I meant CUPS debug logs as logs from cupsd daemon. I would like to see what happens in cupsd, because the issue happens when cups-browsed sends request to delete printer to cupsd. It is just to be sure, because IMHO more we can see from CUPS debug printfs, but that's more complicated to turn on - CUPS has to be compiled to use them, and then the application using libcups has to have several enviroment variables to get debug printfs. If you can reconfigure, recompile CUPS and set cups-browsed to run with those environment variables right now, it would be great to do so - they are CUPS_DEBUG_LEVEL (value 0 to 9), CUPS_DEBUG_FILTER (regexp for selecting specific messages - usually ".*"), CUPS_DEBUG_LOG (path where logs will be saved).

* Shouldn't I  put another debug statement into the `while (lineptr < lineend) {` loop writing the value of` http->used`?

That will be interesting to add when you enable debug printf support - because debug logs from libcups are not going into cupsd logs.

* Can the debug level be raised while cusp-browsed is running or does it have to be switched on before starting the service?

cupsd and cups-browsed are separate daemons, so debug level in cupsd can be raised without restarting cups-browsed. However, if both daemons run in a system manager like systemd, cups-browsed will be restarted because cupsd is restarted to load the new configuration.

[michaelrsweet]

I've pushed the fix to master already. Closing...