On Sun, Jan 11, 2009 at 8:46 AM, m. allan noah <kitno455 at gmail.com> wrote: > On Sun, Jan 11, 2009 at 8:10 AM, Mattias Ellert > <mattias.ellert at fysast.uu.se> wrote: >> 22 dec 2008 kl. 02.31 skrev m. allan noah: >> >>>> On Tue, Dec 9, 2008 at 10:48 AM, Mattias Ellert >>>> <mattias.ellert at fysast.uu.se> wrote: >>>>> >>>>> m?n 2008-12-08 klockan 09:46 -0500 skrev m. allan noah: >>>>>> >>>>>> After some private mails with Ian, it seems this is a bug in >>>>>> sane-avision: >>>>>> >>>>>> during sane_cancel(), the backend calls: sanei_thread_kill >>>>>> (s->reader_pid), but s->reader_pid is 0, which signals the entire >>>>>> group. There is a test to try and avoid this, but it relies on prior >>>>>> code to have set s->reader_pid = -1, which has not happened in the >>>>>> case of no paper. >>>>>> >>>>>> I just expanded the test to require a positive value, since the pid >>>>>> should never be negative anyway? My fix has just been commited to CVS >>>>>> (backend version 289 nice round number for Ford and Studebaker fans). >>>>>> Ian and Rene- please test. >>>>>> >>>>>> allan >>>>> >>>>> This breaks the MacOS X port. The PID number (being a pointer) can be >>>>> arbitrary large, and when cast to an integer it can easily overflow to a >>>>> negative value. The code was fixed for this problem by removing all >>>>> places where the code was checking for a PID > 0. For the avision >>>>> backend this was done here: >>>>> >>>>> >>>>> https://alioth.debian.org/plugins/scmcvs/cvsweb.php/sane-backends/backend/avision.c.diff?r1=1.38;r2=1.39;cvsroot=sane >>>>> >>>>> Your commit: >>>>> >>>>> >>>>> https://alioth.debian.org/plugins/scmcvs/cvsweb.php/sane-backends/backend/avision.c.diff?r1=1.43;r2=1.44;cvsroot=sane >>>>> >>>>> reintroduces the problem fixed by the earlier commit. Please revert it >>>>> and fix the new problem in a way that doesn't break the MacOS X port. >>> >>> Ok, so what is the correct fix? If OSX is using pthread, is it enough >>> to make SANE_Pid pthread_t? >>> >>> allan >> >> >> The SANE_Pid is properly declared as: >> >> #ifdef USE_PTHREAD >> typedef long SANE_Pid; >> #else >> typedef int SANE_Pid; >> #endif >> >> This gives the correct size for both fork and pthread on both 32 and 64 bit. >> Changing it to pid_t and pthread_t instead of int and long would mean an >> interface change (and we get into the change soname or not discussion again) >> - you would also loose the abstraction achieved by using an opaque type in >> the SANE API rather than the implementation specific types. > > Correct me if i am wrong, but we are talking about sanei here, not the > sane API. None of this is in the API. > >> Also since the SANE API states that a SANE_Pid value of -1 indicates an >> error, the SANE_Pid must be a signed type. > > Where does it state this? I dont see SANE_Pid anywere in the API. > > Changing it to pthread_t (which >> essentially is a pointer - hence an unsigned type) will break the API badly. >> Any value for a unsigned type will always be different from -1 (a good >> compiler will optimise the check away). >> >> The only thing that must remembered is that negative values for SANE_Pid are >> valid (except for -1). You can not check for a valid SANE_Pid with (pid > >> 0). > > Pointers could wrap to -1 as well. This fix is not sufficient. I think > we can correct this the right way in sanei.
Ok, I've done a little bit of digging, and it appears that we can fix this by making SANE_Pid an int which we use as an index into an array of platform-specific types, like pthread_t or such. Then we can specifically disallow certain values like anything < 1. comments? allan -- "The truth is an offense, but not a sin"
