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

[m. allan noah]

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"