airscan.c: don't call sane_exit() within sane_init(), return status

[zdohnal]

Hi Alex,

there is an issue in Fedora - perl-Image-Sane, which depends on all SANE backends, fails to build after adding sane-airscan project into Fedora - to be precise, its test suite started to fail.

Petr Pisar, perl-Image-Sane maintainer, was able to isolate the issue to simple scanimage call within mock.

I looked into the issue and found out airscan backend calls sane_exit() within sane_init() if status isn't good. sane_exit() is not linked to sane_exit() within airscan backend, but sane_exit() from sane-backends-libs is called instead.

Calling sane-backends sane_exit() causes to clean up all other opened backends under scanimage hands, resulting into segmentation fault.

I took the solution from pixma backend - it seems the backend doesn't call sane_exit() in case of error, but only returns the current status to function above, which should be capable to clean up (calls sane_exit() on all backends).

With the fix, segfault is gone and perl-Image-Sane test suite passes.

Is the patch okay? Please let me know if I should change something.

If it is okay, would you mind merging it?

Thank you in advance,

Zdenek

[alexpevzner]

Hi Zdenek,

this is not safe, because at this case nobody will cleanup after partial init.

I instead renamed sane_exit() into unexported sane_exit_internal() and call it from the failed sane_init(), so dynamic linker cannot break my game anymore :-)

Please confirm that it fixes the initial issue.

[zdohnal]

The crash moved a little further:

(gdb) 
68	        sane_exit_internal();
(gdb) s
sane_exit_internal () at airscan.c:88
88	    log_debug(NULL, "sane_exit() called");
(gdb) l
83	/* Exit the backend -- internal version
84	 */
85	static void
86	sane_exit_internal (void)
87	{
88	    log_debug(NULL, "sane_exit() called");
89	
90	    eloop_thread_stop();
91	
92	    mdns_cleanup();
(gdb) n
90	    eloop_thread_stop();
(gdb) 
92	    mdns_cleanup();
(gdb) 
93	    wsdd_cleanup();
(gdb) 
file airscan-wsdd.c: line 1696 (wsdd_cleanup): assertion failed: (ll_empty(&wsdd_finding_list))

Thread 1 "scanimage" received signal SIGABRT, Aborted.
0x00007ffff7d75bc5 in raise () from /lib64/libc.so.6
(gdb)

Thread 1 "scanimage" hit Breakpoint 1, wsdd_cleanup () at airscan-wsdd.c:1674
1674	    if (wsdd_netif_notifier != NULL) {
(gdb) n
1679	    for (addr = wsdd_netif_addr_list; addr != NULL; addr = addr->next) {
(gdb) 
1684	    wsdd_netif_addr_list = NULL;
(gdb) 
1686	    if (wsdd_mcsock_ipv4 >= 0) {
(gdb) 
1691	    if (wsdd_mcsock_ipv6 >= 0) {
(gdb) 
1696	    log_assert(wsdd_log, ll_empty(&wsdd_finding_list));
(gdb) 
file airscan-wsdd.c: line 1696 (wsdd_cleanup): assertion failed: (ll_empty(&wsdd_finding_list))

Thread 1 "scanimage" received signal SIGABRT, Aborted.
0x00007ffff7d75bc5 in raise () from /lib64/libc.so.6
(gdb)

[alexpevzner]

Oops! My fault!

Fixed, please, test again.

[zdohnal]

Np, I'll check it the fix.

[zdohnal]

@alexpevzner unfortunately, it is no use unless you initialize the wsdd_log global var, because you check uninitialized var in wsdd_cleanup() otherwise. I updated the commit.

[zdohnal]

Heh, github automatically closes the merge request if the patched code is removed from branch... I'll do a separate pull request.

[alexpevzner]

it is no use unless you initialize the wsdd_log global var

It needs to be better investigated. C zero-fills all uninitialized static variables, and if explicit initialization changes behaviour, something really weird happens (for example, explicit initialization may move variable to another address, from .bss to .data section, and if there is memory corruption error, it may affect another set of variables).

Unfortunately, I can't reproduce.

[zdohnal]

Unfortunately, I can't reproduce.

You can reproduce by gdb. Run scanimage in gdb, break on sane_init() from airscan, go step by step till mdns_init(), set status variable to 1 after mdns_init() and continue.

Steps:

Breakpoint 1, main (argc=1, argv=0x7fffffffe618) at scanimage.c:1999
1999	{
(gdb) n
....
(gdb) 
2324		  status = sane_get_devices (&device_list, SANE_FALSE);

(gdb) s
sane_get_devices (dl=0x7fffffffc1b8, local=0) at dll-s.c:21
21	  return ENTRY(get_devices) (dl, local);
(gdb) s
sane_dll_get_devices (device_list=0x7fffffffc1b8, local_only=0) at dll.c:1064
1064	{
(gdb) 
1085	  DBG (3, "sane_get_devices\n");
(gdb) n
1087	  if (devlist)
(gdb) 
1090	  devlist_len = 0;
(gdb) 
1092	  for (be = first_backend; be; be = be->next)
(gdb) 
1094	      if (!be->inited)
(gdb) 
1095		if (init (be) != SANE_STATUS_GOOD)
... (continue till (*be)->name is 'airscan')
(gdb) s
init (be=be@entry=0x55555556c510) at dll.c:640
640	{
(gdb) 
644	  if (!be->loaded)
(gdb) n
646	      status = load (be);
(gdb) 
647	      if (status != SANE_STATUS_GOOD)
(gdb) 
651	  DBG (3, "init: initializing backend `%s'\n", be->name);
(gdb) 
653	  status = (*(op_init_t)be->op[OP_INIT]) (&version, auth_callback);
(gdb) s
sane_init (version_code=0x7fffffffc044, authorize=0x55555555d6a0 <auth_callback>) at airscan.c:24
24	{
(gdb) n
27	    log_init(); /* Must be the first thing to do */
(gdb) 
28	    trace_init();
(gdb) 
29	    log_debug(NULL, "sane_init() called");
(gdb) 
31	    devid_init();
(gdb) 
32	    conf_load();
(gdb) 
34	    log_configure(); /* As soon, as configuration is available */
(gdb) 
36	    if (version_code != NULL) {
(gdb) 
37	        *version_code = SANE_VERSION_CODE (SANE_CURRENT_MAJOR,
(gdb) 
44	    status = eloop_init();
(gdb) 
46	        status = rand_init();
(gdb) 
49	        status = http_init();
(gdb) 
52	        status = device_management_init();
(gdb) 
55	        status = netif_init();
(gdb) 
58	        status = zeroconf_init();
(gdb) 
55	        status = netif_init();
(gdb) 
58	        status = zeroconf_init();
(gdb) 
61	        status = mdns_init();
(gdb) set var status=1
(gdb) c

[zdohnal]

I'll check what it is really in wsdd_log which causes not being equal to NULL. I will need to rebuild with -O0 - I'm not sure what option - -O0 or -O2 - is preferred if both of them are in CFLAGS, but it would be good that airscan wouldn't hard-code it :( .

[zdohnal]

Hmm... now I'm unable to reproduce too. I'll check perl-Image-Sane test suite in copr just for sure.

[zdohnal]

The build succeeded... I had probably old package in chroot or built without the patch... sorry for the noise, ending of week...

[zdohnal]

@alexpevzner Either way, thanks for the fix!

[alexpevzner]

Nice to have this problem resolved. Thank a lot for your help!