[sane-devel] 39ceeae6 breaks md5 auth

[Olaf Meeuwissen]

Hi James,

James Ring writes:

> Hi Olaf,
>
> On Tue, Jan 2, 2018 at 11:15 PM, Olaf Meeuwissen
> <paddy-hack at member.fsf.org> wrote:
>> Hi James,
>>
>> Thanks for the report.
>>
>> James Ring writes:
>>
>>> Confirmed that with the offending patch, md5.c produces incorrect
>>> digests for known input/output pairs. We should roll it back.
>>>
>>> Also I couldn't reproduce (with gcc 7.2.0) the compiler warning that
>>> the original change was supposed to fix.
>>
>> Hmm, neither can I.  In neither the debian-8-mini nor debian-9-mini CI
>> environments.  But you can still see it in the CI logs for the last
>> pipeline that ran before the offending commit.
>>
>>   https://gitlab.com/sane-project/backends/pipelines/4150861
>>
>> Only the fedora-24-clang log doesn't have it.
>
> Unrelated note: if I read those logs correctly, the CI doesn't run
> `make check`, which maybe they should. It would also be nice to have a
> test case that exercises authentication, though I suspect it is an
> infrequently used feature. I tried adding one, but I couldn't figure
> out how to update the makefiles to include my test.

A `make check` requires /dev/usb to be present in the Docker container
used for the builds in order to pass.  While I have no problem doing
that locally, I haven't tried (or don't recall doing so ;-) on the
shared GitLab.com CI runners.  The VMs that these run on may not even
*have* /dev/usb.

A for an auth test case, a patch with the test is welcome.  I can update
the automake files to integrate the test in the `make check` target.

>>> On Tue, Jan 2, 2018 at 9:57 AM, James Ring <sjr at jdns.org> wrote:
>>>> [...snip...]
>>>>
>>>> Reverting that commit restores the functionality. I haven't figured
>>>> out what the problem is from a cursory inspection of the code, I'll
>>>> continue staring at it.
>>
>> I was about to revert the commit but looking at it now I'm wondering
>> what I was thinking when I committed that :-(  Changing the pointer
>> type to something of a different size *obviously* screws up the array
>> indexing!
>>
>> I've cooked up a fix for that (based on e895ee55).  Could you give the
>> attached patch a try?
>
> Yes, this works! I can't help wondering if it might be better to
> introduce an external dependency on, e.g. libxcrypt or libssl to
> implement md5? That way the sane project is not maintaining an md5
> fork.

Thanks for the testing.  Patch has been pushed.

As for the alternative libraries, thanks for the suggestion.  I noticed
that there is also libcrypt which comes with glibc.  Not sure if there
is anything similar for musl or any of the other OSs that are nominally
supported by sane-backends.  At present, it is less work to maintain the
included code than switch to some "standard" library.

Hope this helps,
--
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Software                        https://my.fsf.org/donate
 Join the Free Software Foundation              https://my.fsf.org/join