[sane-devel] Avision bug (was: Re: Suicidal Child Process - SANE)

Mattias Ellert mattias.ellert at fysast.uu.se
Tue Jan 27 07:51:29 UTC 2009


This one bounced, trying to resend.

	Mattias

-------- Vidarebefordrat meddelande --------
Från: Mattias Ellert <mattias.ellert at fysast.uu.se>
Till: m.allann <oah kitno455 at gmail.com>
Kopia: SANE <sane-devel at lists.alioth.debian.org>
Ämne: Re: [sane-devel] Avision bug (was: Re: Suicidal Child Process -
SANE)
Datum: Sun, 25 Jan 2009 17:11:50 +0100

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 2272 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/sane-devel/attachments/20090127/8adaf4e9/attachment-0001.bin 


More information about the sane-devel mailing list