Bug#1088784: libqqwing2v5: ABI break causes gnome-sudoku to crash after setting difficulty

[Simon McVittie]

Control: forcemerge 1088784 1087397 1086994
Control: retitle 1088784 libqqwing2v5: ABI break causes gnome-sudoku to crash after setting difficulty
Control: reassign 1088784 libqqwing2v5 1.3.4-2
Control: severity 1088784 serious
Control: affects 1088784 + gnome-sudoku

On Sun, 01 Dec 2024 at 14:25:11 +0330, Danial Behzadi wrote:
> After selecting puzzle difficulty and pressing "Startr Game" button,
> [gnome-sudoku] will crash with a "Segmentation fault".

This crash appears to have been caused by an unintended ABI break in
libqqwing2v5. The patch proposed on #1013230 and applied in 1.3.4-2
added a std::mt19937 object to the members of SudokuBoard, increasing
the size of a SudokuBoard, which causes a heap overflow when an older
build of gnome-sudoku only allocates enough space for the old size of
SudokuBoard.

One way to see the ABI break is to build both 1.3.4-1.1 and 1.3.4-2 with
DEB_BUILD_OPTIONS=nostrip, and then use abidiff(1) from the abigail-tools
package to compare them. It reports, among other things:

> $ abidiff --show-bytes \
>   ~/tmp/deb/build/qqwing-1.3.4-1.1/.libs/libqqwing.so.2.1.0 \
>   ~/tmp/deb/build/qqwing-1.3.4-2/.libs/libqqwing.so.2.1.0
...
> 1 function with some indirect sub-type change:
>
>   [C] 'method qqwing::SudokuBoard::SudokuBoard()' at qqwing.cpp:111:1 has some indirect sub-type changes:
>     implicit parameter 0 of type 'qqwing::SudokuBoard*' has sub-type changes:
>       in pointed to type 'class qqwing::SudokuBoard' at qqwing.hpp:50:1:
>         type size changed from 80 to 5080 (in bytes)
>         1 data member insertion:
>           'std::mt19937 rand_generator', at offset 0 (in bytes) at qqwing.hpp:130:1
>         12 data member changes:
>           'int* puzzle' offset changed from 0 to 5000 (in bytes) (by +5000 bytes)
>           'int* solution' offset changed from 8 to 5008 (in bytes) (by +5000 bytes)
>           'int* solutionRound' offset changed from 16 to 5016 (in bytes) (by +5000 bytes)
>           'int* possibilities' offset changed from 24 to 5024 (in bytes) (by +5000 bytes)
>           'int* randomBoardArray' offset changed from 32 to 5032 (in bytes) (by +5000 bytes)
>           'int* randomPossibilityArray' offset changed from 40 to 5040 (in bytes) (by +5000 bytes)
>           'bool recordHistory' offset changed from 48 to 5048 (in bytes) (by +5000 bytes)
>           'bool logHistory' offset changed from 49 to 5049 (in bytes) (by +5000 bytes)
>           'vector<qqwing::LogItem*, std::allocator<qqwing::LogItem*> >* solveHistory' offset changed from 56 to 5056 (in bytes) (by +5000 bytes)
>           'vector<qqwing::LogItem*, std::allocator<qqwing::LogItem*> >* solveInstructions' offset changed from 64 to 5064 (in bytes) (by +5000 bytes)
>           'qqwing::SudokuBoard::PrintStyle printStyle' offset changed from 72 to 5072 (in bytes) (by +5000 bytes)
>           'int lastSolveRound' offset changed from 76 to 5076 (in bytes) (by +5000 bytes)

(Yes, adding a data member to an exported class is an ABI break, even if
the member is declared to be private! This is unfortunate, but it's
how C++ works.)

If the upstream developer breaks the ABI of the library, they would have
to bump the major ABI version that is part of the SONAME (currently
2), and the Debian maintainer would respond to that by carrying out
a transition in coordination with the release team. The ABI and the
sequence of SONAME major versions are "owned" by the upstream developer,
and Debian-specific ABIs should be avoided.

I think that particular patch should not have been applied as a
Debian-specific change, and especially not during a "salvaging"
process that does not result in the package having an active
maintainer. Significant changes that do not have any reason to be
Debian-specific should either go upstream to
https://github.com/stephenostermiller/qqwing, or not be applied at all.

As a short-term solution to this, I think a games team member should
revert the addition of that patch, reopening #1013230. That would restore
the ABI of libqqwing.so.2 to the state that is present upstream and in
Debian 12.

In the longer term, if randomization improvements can be made *without*
breaking the ABI, preferably in consultation with the upstream developer,
that would be ideal. A simple way to do this would be to make main.cpp
use applicationStartTime as part of the input to srand(), assuming that
two instances of the program will not usually be instantiated during
the same microsecond. Or, the std::mt19937 object could be kept, but
made into a file-scoped global or thread-local variable instead of an
object member - I don't know whether this particular library is designed
to be thread-safe, and I also don't know whether a std::mt19937 object
is safe to share between threads, so I don't know precisely what would
be required here.

Or, if improvements to the library require breaking the ABI, then that
should be done **in consultation with upstream** so that they can carry
out an intentional SONAME bump (setting QQWING_CURRENT, QQWING_REVISION,
QQWING_AGE appropriately), after which gnome-sudoku can be rebuilt in
a coordinated way against the new SONAME.

I am not currently intending to upload qqwing myself, because it
currently has no Uploaders listed, which means that next time it is
uploaded, either a games team member (possibly Andreas) needs to take
responsibility for it by adding themselves to Uploaders, or the package
needs to be orphaned again.

    smcv