On Sun, Jan 25, 2009 at 11:11 AM, Mattias Ellert <mattias.ellert at fysast.uu.se> wrote: > > 15 jan 2009 kl. 03.09 skrev m. allan noah: > >> 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. > > Yes, it is not in the SANE client API. It is in sanei, which is part of the > API for writing SANE backends. Sorry for being unclear, but you seem to have > got what I meant anyway. > >>>> 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? > > I am not sure what you are trying to achieve. I am perfectly happy with the > current implementation of sanei-thread. I just pointed out that your latest > change to your backend code violates this current implementation. If you > insist on your changes to your backend code the sanei-thread implementation > must be changed to allow your backend to run, but doesn't it make more sense > to make your backend compatible to the current sanei-thread implementation > rather than doing it the other way around? > > Mattias > >
ok, I'll say it a third time, but phrase it as a question: Could this pointer also wrap to -1 as well? allan -- "The truth is an offense, but not a sin"