[PATCH v4 0/2] MR9217: dlls/twain_32: Fix segfault when application unloads and reloads twain_32.dll
# Fault secnario If an application loads twain_32.dll for every scan and unloads it when done with scanning, it very likely crashes when the user chooses to scan a second time within the same process. Unloading twain_32.dll is not uncommon behaviour. # Internal error cause When sane.ds gets loaded the first time, it retrieves the address of the DSM_Entry function from twain_32.dll and stores it in a variable SANE_dsmentry in the sane.ds global data. When the DS gets closed, the sane.ds does not get unloaded due to an old workaround in dlls/twain_32/dsm_ctrl.c:275: ``` twRC = currentDS->dsEntry (pOrigin, DG_CONTROL, DAT_IDENTITY, MSG_CLOSEDS, pData); /* This causes crashes due to still open Windows, so leave out for now. * FreeLibrary (currentDS->hmod); */ ``` When the application program closes the DSM (twain_32.dll) and reloads it, there is a hight probability that it will be loaded in a different memory location. So the exported DSM_Entry function is located at a different address. But the code in dlls/sane.ds/sane_main.c:60 does not update the pointer if SANE_dsmentry is not NULL: ``` if (SANE_dsmentry == NULL) { HMODULE moddsm = GetModuleHandleW(L"twain_32"); if (moddsm) SANE_dsmentry = (void*)GetProcAddress(moddsm, "DSM_Entry"); ``` So SANE_dsmentry still points to where the twain_32.dll was loaded the first time it loaded the sane.ds. Which is now an invalid address. When later the DS wants to notify the application of an event (DS Closed, Image transfered..) it calls SANE_Notify in sane_main.c:357: ``` void SANE_Notify (TW_UINT16 message) { SANE_dsmentry (&activeDS.identity, &activeDS.appIdentity, DG_CONTROL, DAT_NULL, message, NULL); } ``` And here the crash occurs due to the invalid pointer in variable SANE_dsmentry. # Suggested Solution There are many ways how this problem could be solved. The solution suggested here is based on an extension of the TWAIN protocol in Version 2: The DSM transfers the entry points to it's callback functions in a message: DG_CONTROL / DAT_ENTRYPOINT / MSG_SET That message was already implemented in sane.ds and gphoto2.gs. And both also set the flag DF_DS2 in the identitiy information SupportedGroups, so the DSM knows it is allowed to send that message. So this approach is not a workaround, but an additional feature that solves the problem in the way intended according to the TWAIN specification. This also means that the patch should solve the problem in closed source TWAIN drivers a user might install. However [sane.d](http://sane.de)s and gphoto2.ds do not fill the TW_IDENTITY information in the response to the DAT_IDENTITIY / MSG_OPENDS message. This is different from the behaviour observed on windows. For that reason the DF_DS2 flag in SupportedGroups is also not set, and thus a change in sane.ds is neccessary to make the DAT_ENTRYPOINT solution work. This is also not just a workaround but a change of the behaviour so it becomes closer to what is observed on windows. # Test scenario software I've written a small 163 lines C application program to reproduce the erroneous behaviour. It is called unloaddsm.c and I'll attach it to this merge request. It is meant to be cross-compiled with mingw-w64. It loads the DSM, opens the DS (withour showing the UI), closes the DS, unloads DSM, re-loads DSM, re-opens the DS and then displays the UI (User Interface) when the user then clicks "cancel" or presses escape, this causes the call of SANE_Notify that leads to the crash. It also shows the TW_IDENTITY fields that sane.ds did not fill. I ran the same program with a 32-Bit windows TWAIN driver on Windows 10 and there the idendity fields were filled. With the patch applied, the error does not occur any more in sane.ds. ``` Archive: /tmp/unloaddsm.zip Length Date Time Name 5283 2025-10-18 23:31 unloaddsm.c 77104 2025-10-16 14:02 twain.h 161 2025-10-18 15:57 Makefile ``` -- v4: dlls/sane.ds: when opening a DS, return the identitity information of the opened device to the caller. Matches windows behaviour and neccessary for the change in dsm_ctrl.c to work dlls/twain_32: Avoid crash when application unloads twain32_dll and reloads it for a second scan by implementing TWAIN feature DG_CONTROL / DAT_ENTRYPOINT / MSG_SET https://gitlab.winehq.org/wine/wine/-/merge_requests/9217
From: Bernd Herd <codeberg(a)herdsoft.com> --- dlls/twain_32/dsm_ctrl.c | 64 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/dlls/twain_32/dsm_ctrl.c b/dlls/twain_32/dsm_ctrl.c index 13503e6772c..b99ed3dabf2 100644 --- a/dlls/twain_32/dsm_ctrl.c +++ b/dlls/twain_32/dsm_ctrl.c @@ -118,6 +118,60 @@ twain_autodetect(void) { #endif } +/** + * Callback function used in TW_ENTRYPOINT as defined by the TWAIN Group + * @param _size Size for the newly allocated global memory block + * @return new GlobalAlloc memory handle + */ +static TW_HANDLE PASCAL _DSM_MemAllocate(TW_UINT32 _size) +{ + return GlobalAlloc(GMEM_MOVEABLE, _size); +} + + +/** + * Callback function used in TW_ENTRYPOINT as defined by the TWAIN Group + * @param _handle Handle created by DSM_MemAllocate that shall now be freed + */ +static void PASCAL _DSM_MemFree(TW_HANDLE _handle) +{ + GlobalFree(_handle); +} + + +/** + * Callback function used in TW_ENTRYPOINT as defined by the TWAIN Group + * @param _handle Handle created by DSM_MemAllocate that shall now be + * locked at a certain address + * @return The current address to the data contained in this handle + */ +static TW_MEMREF PASCAL _DSM_MemLock(TW_HANDLE _handle) +{ + return GlobalLock(_handle); +} + + +/** + * Callback function used in TW_ENTRYPOINT as defined by the TWAIN Group + * @param _handle Handle created by DSM_MemAllocate that shall now be + * unlocked to allow the memory handler to move it to + * different addresses, + */ +static void PASCAL _DSM_MemUnlock(TW_HANDLE _handle) +{ + GlobalUnlock(_handle); +} + +/// Structure with pointers being transfered in DG_CONTROL / DAT_ENTRYPOINT / MSG_SET to the data source +static const TW_ENTRYPOINT _entrypoints = { + .Size = sizeof(TW_ENTRYPOINT), + .DSM_Entry = DSM_Entry, + .DSM_MemAllocate = _DSM_MemAllocate, + .DSM_MemFree = _DSM_MemFree, + .DSM_MemLock = _DSM_MemLock, + .DSM_MemUnlock = _DSM_MemUnlock +}; + /* DG_CONTROL/DAT_NULL/MSG_CLOSEDSREQ|MSG_DEVICEEVENT|MSG_XFERREADY */ TW_UINT16 TWAIN_ControlNull (pTW_IDENTITY pOrigin, pTW_IDENTITY pDest, activeDS *pSource, TW_UINT16 MSG, TW_MEMREF pData) { @@ -342,6 +396,16 @@ TW_UINT16 TWAIN_OpenDS (pTW_IDENTITY pOrigin, TW_MEMREF pData) newSource->event_window = NULL; activeSources = newSource; DSM_twCC = TWCC_SUCCESS; + + /* Tell the source our entry points */ + if (pIdentity->SupportedGroups & DF_DS2) { + /* This makes sure that the DS knows the current address of our DSM_Entry + * function so there is no risc that it is using a stale copy. + * The other entry points are also set for formal reasons, + * but are currently not used. + */ + newSource->dsEntry (pOrigin, DG_CONTROL, DAT_ENTRYPOINT, MSG_SET, (TW_ENTRYPOINT *) &_entrypoints); + } return TWRC_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9217
From: Bernd Herd <codeberg(a)herdsoft.com> --- dlls/sane.ds/unixlib.c | 16 +++++++++++++--- dlls/twain_32/dsm_ctrl.c | 6 ++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/dlls/sane.ds/unixlib.c b/dlls/sane.ds/unixlib.c index 3ced42df1fb..861e243a215 100644 --- a/dlls/sane.ds/unixlib.c +++ b/dlls/sane.ds/unixlib.c @@ -266,10 +266,20 @@ static NTSTATUS open_ds( void *args ) return STATUS_DEVICE_NOT_CONNECTED; } status = sane_open( device_list[i]->name, &device_handle ); - if (status == SANE_STATUS_GOOD) return STATUS_SUCCESS; + if (status != SANE_STATUS_GOOD) { + ERR("sane_open(%s): %s\n", device_list[i]->name, sane_strstatus (status)); + return STATUS_DEVICE_NOT_CONNECTED; + } + + /* return the Identity of the device opened to the caller */ + id->ProtocolMajor = TWON_PROTOCOLMAJOR; + id->ProtocolMinor = TWON_PROTOCOLMINOR; + id->SupportedGroups = DG_CONTROL | DG_IMAGE | DF_DS2; + copy_sane_short_name(device_list[i]->name, id->ProductName, sizeof(id->ProductName) - 1); + lstrcpynA (id->Manufacturer, device_list[i]->vendor, sizeof(id->Manufacturer) - 1); + lstrcpynA (id->ProductFamily, device_list[i]->model, sizeof(id->ProductFamily) - 1); - ERR("sane_open(%s): %s\n", device_list[i]->name, sane_strstatus (status)); - return STATUS_DEVICE_NOT_CONNECTED; + return STATUS_SUCCESS; } static NTSTATUS close_ds( void *args ) diff --git a/dlls/twain_32/dsm_ctrl.c b/dlls/twain_32/dsm_ctrl.c index b99ed3dabf2..89e788f979d 100644 --- a/dlls/twain_32/dsm_ctrl.c +++ b/dlls/twain_32/dsm_ctrl.c @@ -162,7 +162,9 @@ static void PASCAL _DSM_MemUnlock(TW_HANDLE _handle) GlobalUnlock(_handle); } -/// Structure with pointers being transfered in DG_CONTROL / DAT_ENTRYPOINT / MSG_SET to the data source +/* Structure with pointers being transfered in + * DG_CONTROL / DAT_ENTRYPOINT / MSG_SET to the data source + */ static const TW_ENTRYPOINT _entrypoints = { .Size = sizeof(TW_ENTRYPOINT), .DSM_Entry = DSM_Entry, @@ -400,7 +402,7 @@ TW_UINT16 TWAIN_OpenDS (pTW_IDENTITY pOrigin, TW_MEMREF pData) /* Tell the source our entry points */ if (pIdentity->SupportedGroups & DF_DS2) { /* This makes sure that the DS knows the current address of our DSM_Entry - * function so there is no risc that it is using a stale copy. + * function so there is no risk that it is using a stale copy. * The other entry points are also set for formal reasons, * but are currently not used. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9217
Esme Povirk (@madewokherd) commented about dlls/twain_32/dsm_ctrl.c:
/* Tell the source our entry points */ if (pIdentity->SupportedGroups & DF_DS2) { /* This makes sure that the DS knows the current address of our DSM_Entry - * function so there is no risc that it is using a stale copy. + * function so there is no risk that it is using a stale copy. Can you move the comment fixes to the first commit, which added the comments, please?
The easiest way to do this is likely * Run `git rebase -i`, marking the first commit as edit * Use `git restore -SW --source=my-tain_unload_fix dlls/twain_32/dsm_ctrl.c` to stage the current version of this file. * `git commit --amend` to update the commit. * `git rebase --continue` to finish the rebase. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9217#note_120160
On Thu Oct 30 19:27:25 2025 +0000, Esme Povirk wrote: > Can you move the comment fixes to the first commit, which added the > comments, please? > The easiest way to do this is likely > * Run `git rebase -i`, marking the first commit as edit > * Use `git restore -SW --source=my-tain_unload_fix > dlls/twain_32/dsm_ctrl.c` to stage the current version of this file. > * `git commit --amend` to update the commit. > * `git rebase --continue` to finish the rebase. Personally I'd follow a different checklist: - Open text editor, revert the comment changes, amend the twain commit - Redo the comment changes (probably via text editor undo), in a separate commit, with a commit message like 'to sane' - rebase -i - Swap the latter two commits, and mark the comment one as fixup Mine needs a bit more text editor work, but fewer long git commands; the result will be the same. Pick your favorite. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9217#note_120177
participants (4)
-
Alfred Agrell (＠Alcaro) -
Bernd Herd
-
Bernd Herd (＠herdsoft)
-
Esme Povirk (＠madewokherd)