toml should be reentrant

[markus2330]

trafficstars

src/plugins/toml/driver.c and src/plugins/toml/lexer.l unfortunately has non-const global variables, which easily can lead to weird memory problems and crashes, sometimes with output:

realloc(): invalid next size
fatal flex scanner internal error--end of buffer missed

Here a valgrind run:

==10740== Invalid read of size 8
==10740==    at 0x48EB196: fread (iofread.c:37)
==10740==    by 0x5FBD3D7: yy_get_next_buffer (lexer.c:1975)
==10740==    by 0x5FBD3D7: yylex (lexer.c:1815)
==10740==    by 0x5FBE163: yyparse (parser.c:1644)
==10740==    by 0x5FB5A4F: driverParse (driver.c:132)
==10740==    by 0x5FB5A4F: tomlRead (driver.c:64)
==10740==    by 0x5FB58EA: elektraTomlGet (toml.c:42)
==10740==    by 0x4E0B8A8: elektraGetDoUpdateWithGlobalHooks (kdb.c:889)
==10740==    by 0x4E0C934: kdbGet (kdb.c:1409)
==10740==    by 0x1990CC: elektra::kdb::KDB::get (kdb.rs:64)
==10740==  Address 0x6186c58 is 136 bytes inside a block of size 472 free'd
==10740==    at 0x48399AB: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==10740==    by 0x48EA2FE: _IO_deallocate_file (libioP.h:863)
==10740==    by 0x48EA2FE: fclose@@GLIBC_2.2.5 (iofclose.c:74)
==10740==    by 0x5FB5A5E: driverParse (driver.c:134)
==10740==    by 0x5FB5A5E: tomlRead (driver.c:64)
==10740==    by 0x5FB58EA: elektraTomlGet (toml.c:42)
==10740==    by 0x4E0B8A8: elektraGetDoUpdateWithGlobalHooks (kdb.c:889)
==10740==    by 0x4E0C934: kdbGet (kdb.c:1409)
==10740==    by 0x1990CC: elektra::kdb::KDB::get (kdb.rs:64)
==10740==  Block was alloc'd at
==10740==    at 0x483877F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==10740==    by 0x48EABAA: __fopen_internal (iofopen.c:65)
==10740==    by 0x5FB5A33: driverParse (driver.c:125)
==10740==    by 0x5FB5A33: tomlRead (driver.c:64)
==10740==    by 0x5FB58EA: elektraTomlGet (toml.c:42)
==10740==    by 0x4E0B8A8: elektraGetDoUpdateWithGlobalHooks (kdb.c:889)
==10740==    by 0x4E0C934: kdbGet (kdb.c:1409)
==10740==    by 0x1990CC: elektra::kdb::KDB::get (kdb.rs:64)

The global variables should be removed and passed via a plugin-specific struct instead. (In general non-const global variables are not allowed in Elektra, this was an oversight in the code review.)

[kodebach]

AFAICT all the globals currently used come from Bison/Yacc (they are all yy*). At least for modern Bison there seem to be ways to generate a fully reentrant parser which should avoid these globals.

@markus2330 I assume this issues is intended for FLOSS? If so, we should probably label it.

[markus2330]

@kodebach thank you, very good input!

I assume this issues is intended for FLOSS?

As you clarified that Bison has support for it, it can actually be a FLOSS issue. I labelled it.

[markus2330]

@kodebach this should be part of #2330

[kodebach]

#2330 is closed, did you mean #3910 or maybe #3490? If so, you should probably also link it from there.

[markus2330]

Thank you for telling me, GitHub auto completion played a joke on me, I added the task.

[flo91]

Completed by #4869

[markus2330]

@flo91 did you test it? Probably also another lexer function needs to be used?

[flo91]

@markus2330 How can this be tested properly? Is there a specific test case / procedure? Unfortunately, I don't have any experience with the toml-plugin.

I've just noticed that in https://github.com/ElektraInitiative/libelektra/pull/4872/commits/8f7142fb059215cf0b09135633b4e10a07d09286 the files driver.c and lexer.l, which are also mentioned in the first posting of this issue, were updated.

Furthermore, @kodebach wrote in the release-notes:

The flex lexer and bison parser are now fully reentrant and therefore thread-safe.

That sounded to me like it should solve this issue.

[markus2330]

I tried to mount a toml plugin to user:/test/race and run kdb race 1 20 1 (to test with 20 threads) and there was no problem. Furthermore, %define api.pure full also seems to be added. So probably it is fixed.