Prevent deleting a printer that is in use - #1655
Conversation
If the thread create_local_bg_thread is currently using a printer but the printer takes longer than 300 seconds to respond, the printer will get deleted by cupsdDeleteTemporaryPrinters. If the printer then responds, the background thread will be referencing an invalid pointer. Add a helper method to check to see if any background thread is using a printer before deleting it.
michaelrsweet
left a comment
michaelrsweet
left a comment
There was a problem hiding this comment.
I agree we should do something about this, but I'm concerned about the performance of your proposed "printer is in use by client" function.
michaelrsweet
added
bug
|
I considered creating a counter for the printer in use, or maybe just a bool would be good enough. But then we just need to make sure it's thread safe. I thought the 'is printer in use' function might be simpler, though not as efficient. I can get another option together and see what you think. |
If the thread create_local_bg_thread is currently using a printer but the printer takes longer than 300 seconds to respond, the printer will get deleted by cupsdDeleteTemporaryPrinters. If the printer then responds, the background thread will be referencing an invalid pointer. Add a counter to the printer struct to keep track of how many background threads are using it. Only delete a printer if it's unused.
| for (p = (cupsd_printer_t *)cupsArrayFirst(Printers); p; p = (cupsd_printer_t *)cupsArrayNext(Printers)) | ||
| { | ||
| if (p->temporary && !printer_in_use_by_client(p) && | ||
| if (p->temporary && p->use == 0 && |
There was a problem hiding this comment.
I didn't do any locking here since we don't use it for p->state_time either - is that OK?
There was a problem hiding this comment.
Yes, given the limited use of threading it isn't necessary.
|
Updated this to do reference counting on the printer. If you like this better, I can squash and force-push. Just let me know. Thanks. |
| for (p = (cupsd_printer_t *)cupsArrayFirst(Printers); p; p = (cupsd_printer_t *)cupsArrayNext(Printers)) | ||
| { | ||
| if (p->temporary && !printer_in_use_by_client(p) && | ||
| if (p->temporary && p->use == 0 && |
There was a problem hiding this comment.
Yes, given the limited use of threading it isn't necessary.
2 participants
If the thread create_local_bg_thread is currently using a printer but the printer takes longer than 300 seconds to respond, the printer will get deleted by cupsdDeleteTemporaryPrinters. If the printer then responds, the background thread will be referencing an invalid pointer.
Add a helper method to check to see if any background thread is using a printer before deleting it.