Devilutionx: potential overflow in use of strcpy

[Tmkrth]

Hello! Thank you for devilutionx!

I bought a bow from Griswold, it needs 70 dexterity, which are available. But it cannot be equipped.

First image shows the requirement

I tried undressing all items and am holding the bow with the mouse and try to equip it, one cannot see that. But the status box indicates it.

To reproduce, here is a save game. The bow in question lies on the ground next to you.
single_1.sv.zip

Regards

[AJenbo]

thanks for the report could you let us know what version this is?

[qndel]

it requires 35 str too :)

[qndel]

[Tmkrth]

Thank you very much!

It's DevilutionX v1.1.0

[qndel]

what platform? I just tested 1.1.0 and I got the same description as on the screenshot above

[Tmkrth]

It is on Linux. The diabdat.mpq is from the GOG version. She was created in 1.1.0, I believe. Could you prove that with your tool?

[qndel]

what?

[Tmkrth]

Prove what? That she was created in 1.1.0.

What is the name of that tool?

[qndel]

WHAT TOOL

[Tmkrth]

Your screenshot. Oh, is it devilutionx itself? I thought it might be an editor software or something for the save files.

[qndel]

no, it was just devilutionx :D and I verified it's the same with 1.1.0 release

[AJenbo]

@Tmkrth where did you get your versions, there a lots of unofficial versions for Linux (what distribution) floating around with various minor issues. Qndel's screen shot is from a debug build of devilutionx with -v enabled (the grid on the ground).

[Tmkrth]

Tried with a higher resolution for I though it may be not enough space to show. Also tried setting different values for the various scaling options. But it still shows wrong.

It's the AUR package https://aur.archlinux.org/packages/devilutionx/

[Tmkrth]

Alright, I built it from here and it shows correctly.

The AUR package installs this tarball https://github.com/diasurgical/devilutionX/archive/1.1.0.tar.gz

Thank you, people!

[AJenbo]

Could you test the official build: https://github.com/diasurgical/devilutionX/releases/download/1.1.0/devilutionx-linux-x86_64.7z

[Tmkrth]

Works fine in both the self built and released version from the link above.

./devilutionx-git --version
DevilutionX v1.1.0-330074e8

./devilutionx-release --version
DevilutionX v1.1.0

[AJenbo]

I guess the AUR package has some kind of modification, maybe you can report the issue to there maintainer

[M0Rf30]

Hi, actually there's no modification in my packaging script:

# Maintainer: robertfoster
# Contributor: LIN Rs <LinRs[d]users.noreply.github.com>
# Contributor: yochananmarqos <mark.wagie at tutanota dot com>

_pkgname=devilutionX
pkgname=devilutionx
pkgver=1.1.0
pkgrel=1
pkgdesc="Diablo devolved for linux"
arch=('armv6h' 'armv7h' 'arm' 'aarch64' 'i686' 'x86_64')
url="https://github.com/diasurgical/devilutionX"
license=('custom:unlicense')
depends=('graphite' 'libsodium' 'sdl2_mixer' 'sdl2_ttf')
makedepends=('cmake' 'gcc-libs')
install="$pkgname".install
options=('strip')
source=("https://github.com/diasurgical/devilutionX/archive/$pkgver.tar.gz")

prepare() {
    cd "$srcdir/${_pkgname}-$pkgver"
    if [ ! -d build ]; then
        mkdir build
    fi
}

build() {
    cd "$srcdir/${_pkgname}-$pkgver/build"
    cmake .. \
        -DCMAKE_INSTALL_PREFIX="$pkgdir/usr" \
        -DPIE=ON \
        -DBINARY_RELEASE=ON \
        -DVERSION_NUM="$pkgver"

    cmake --build .
}

package() {
    cd "$srcdir/${_pkgname}-$pkgver/build"
    cmake --install .
}

md5sums=('76e7f5219e8f58ee71ab671b13ce3139')

It uses official tagged tar.gz. Only PIE flag diverge from devilutionx travis pipeline and, of course, the entire build environment cause of Arch rolling nature. So an unknown bug due to libraries? I'm just assuming

[M0Rf30]

Found the guilty.
_FORTIFY_SOURCE directive (a default CXX Flag for many distros) generate this warning at compile time:

[100%] Linking CXX executable devilutionx
In function ‘strcpy’,
    inlined from ‘AddPanelString’ at /home/gianluca/devilutionx/src/devilutionX-1.1.0/Source/control.cpp:552:8,
    inlined from ‘PrintItemDur’ at /home/gianluca/devilutionx/src/devilutionX-1.1.0/Source/items.cpp:3106:18,
    inlined from ‘CheckInvHLight’ at /home/gianluca/devilutionx/src/devilutionX-1.1.0/Source/inv.cpp:2026:16:
/usr/include/bits/string_fortified.h:90:33: warning: ‘__builtin_memcpy’ writing 15 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
   90 |   return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
      |                                 ^
[100%] Built target devilutionx

It generates a valid build but as @Tmkrth stated, requirements are not correctly printed.
I've fixed the PKGBUILD using "-DCMAKE_CXX_FLAGS_RELEASE="-D_FORTIFY_SOURCE=0"

[M0Rf30]

if could be of interest another warning with standard arch linux flags is this reported below

[ 82%] Building CXX object CMakeFiles/devilutionx.dir/SourceX/DiabloUI/art_draw.cpp.o
/home/devilutionx/src/devilutionX-1.1.0/SourceX/storm/storm_net.cpp: In function ‘dvl::BOOL dvl::SNetUnregisterEventHandler(int, dvl::SEVTHANDLER)’:
/home/devilutionx/src/devilutionX-1.1.0/SourceX/storm/storm_net.cpp:53:50: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   53 |  return dvlnet_inst->SNetUnregisterEventHandler(*(event_type *)&evtype, func);
      |                                                  ^~~~~~~~~~~~~~~~~~~~~
/home/devilutionx/src/devilutionX-1.1.0/SourceX/storm/storm_net.cpp: In function ‘dvl::BOOL dvl::SNetRegisterEventHandler(int, dvl::SEVTHANDLER)’:
/home/devilutionx/src/devilutionX-1.1.0/SourceX/storm/storm_net.cpp:58:48: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   58 |  return dvlnet_inst->SNetRegisterEventHandler(*(event_type *)&evtype, func);
      |                                                ^~~~~~~~~~~~~~~~~~~~~
[ 83%] Building CXX object CMakeFiles/devilutionx.dir/SourceX/DiabloUI/errorart.cpp.o

[rehaby]

Looks like the string is declared for 4 lines, but is used with 5

char panelstr[4][64];

void AddPanelString(const char *str, BOOL just)
{
    strcpy(panelstr[pnumlines], str);
    pstrjust[pnumlines] = just;

    if (pnumlines < 4)
        pnumlines++;
}

I think changing the 4 in the panelstr definition to a 5 might fix this

[qndel]

I don't think so, item name isn't using panelstr so 4 lines fit inside panelstr[4]
would be helpful if I could reproduce the bug somehow but it seems to be exclusive to arch linux 🤷

[M0Rf30]

@qndel It's so simple to reproduce. just use -DCMAKE_CXX_FLAGS_RELEASE="-D_FORTIFY_SOURCE=2"
or

CPPFLAGS="-D_FORTIFY_SOURCE=2"

[qndel]

@qndel It's so simple to reproduce. just use "-DCMAKE_CXX_FLAGS_RELEASE="-D_FORTIFY_SOURCE=1"

Could you join devilution's discord? It's much easier to communicate there in my opinion :p https://discord.gg/xYMGzRtU

[M0Rf30]

Here as a reference the FLAGS used on Arch Linux