[Pkg-cups-devel] Bug#401405: Further information and patch

[Ulrich Eckhardt]

I've been browsing the sources a bit, and the primary case is a bit clearer 
now. In backend/usb-unix.c, in open_device and list_devices is a loop that is 
iterating over all possible USB printers and trying to match them against the 
given URI. It always tries "/dev/usblp%d", "/dev/usb/lp%d" 
and "/dev/usb/usblb%d" for any given index and the error handling here is 
broken - it only reports the errno value of the last attempt to open a 
printer, and since I don't have 16 printers that is always ENOENT. The fact 
that it got an EACCESS while opening /dev/usb/lp0 is totally left out.
I think the algorithm to better diagnose errors goes like this:
1. If all open() calls set errno=ENOENT, signal that no printers were found. 
This could also be done using stat(), no need to call open().
2. If any printers were present but could not be opened, the fact and the 
reason should go into a logfile. If the search does not find the requested 
printer, the fact that not all printers could be probed should be returned 
and a hint to look into the logfile.
3. If you could probe them all, just return whether the requested printer was 
found.

I'm not sure how this should be handled in the case of list_devices, either it 
should simply print five question marks for the five fields it otherwise 
fills with the URI and other data or an error message. I took the easy way 
out using the error message.

I have attached the modified usb-unix.c. Changes/Notes:
- It now correctly gives me an access denied error in the webfrontend and 
works as described above.
- When invoking the backend without arguments, it now either probes the 
printer for its ID or gives an errormessage.
- I marked a bogus check for EBUSY.
- In the whole CUPS sourcecode I found cases where sprintf() was used 
(potential buffer overflows) or snprintf() used wrong (snprintf() can leave 
an unterminated string when the buffer is too small). I'd suggest using an 
xsnprintf()-wrapper, similarly to xalloc() either formatting the string into 
the supplied buffer or invoking exit() - the code isn't prepared to handle 
that error anyway, so there's no way to continue. This last point would merit 
a whole code audit.
- I find it also astonishing how many magic numbers are used, as if using a 
buffer of size 255 or 1024 would magically make your code correct...
- All driver backends seem to take similar arguments, except one of 
them. 'ipp' takes a variable number of files to print. In fact I wonder why 
there is no central main() function for commandline handling etc that then 
only calls the implementations from the particular backend. There's a lot of 
code duplication going on.
- There's a weird mixture of tabs and spaces in the files. I only preserved 
the two spaces indentation depth and didn't use any tabs, but I'd say it 
would be worth running the whole code through an autoindenter.
- There is some code in cups-deviced.c that I don't understand. When invoking 
the backend, it code first checks if either "group" or "others" have 
read/write/execute permissions on the backend and if yes, it calls seteuid() 
with the user ID of 'lp'. So, by just removing read and execute permissions 
on the backend, we can make cupsd call it with root permissions (it then 
fails without meaningful error when invoking foomatic-rip). I find this idea 
flawed. Also, quote from the (default) configfile:
  # Note: the server must be run initially as root to support the
  # default IPP port of 631.  It changes users whenever an external
  # program is run...
IOW, this comment is simply a lie and keeping root privileges is a possible 
security risk. Well, not that anyone sane would be running CUPS in a hostile 
network anyway...

Okay, so much for that. Sorry for drifting of into ranting towards the end, 
but I hope the info was at least helpful. Question on that: is there any 
packaged version of 1.3 already? I would have preferred hacking on that one 
and not on a released version...

Also, you (the maintainers) probably have a connection to the upstream CUPS 
developers, could you ask them if they could enable bug-reporting without 
creating yet another account to log into their website? I find this extremely 
annoying...

so much for today, thank you all for your work

Uli
-------------- next part --------------
A non-text attachment was scrubbed...
Name: usb-unix.c
Type: text/x-csrc
Size: 13647 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/pkg-cups-devel/attachments/20061209/77feb88c/usb-unix.c