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

[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.
>
>> 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"