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

[Mattias Ellert]

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.

Also since the SANE API states that a SANE_Pid value of -1 indicates  
an error, the SANE_Pid must be a signed type. 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).

	Mattias

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1444 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/sane-devel/attachments/20090111/f17343c7/attachment.bin