Skip to content

Fix stringop-overread issue and quality-of-life#1

Open
joergen7 wants to merge 4 commits into
mschlaegl:masterfrom
joergen7:master
Open

Fix stringop-overread issue and quality-of-life#1
joergen7 wants to merge 4 commits into
mschlaegl:masterfrom
joergen7:master

Conversation

@joergen7

Copy link
Copy Markdown

This pr contributes a few quality-of-life improvements that I have come across. It fixes a stringop-overread warning issued by the C compiler at build, sets the minimum cmake version to a non-deprecated version, and replaces a deprecated call to distutils.

Fix stringop-overread Warning

Observation

When building pylibfst the C-compiler outputs the following warning:

In function ‘fstWritex’,
    inlined from ‘fstReaderIterBlocks2.part.0’ at /tmp/build-via-sdist-b4bq_3__/pylibfst-0.2.1.dev0/fst/fstapi.c:5903:41:
/tmp/build-via-sdist-b4bq_3__/pylibfst-0.2.1.dev0/fst/fstapi.c:3468:21: warning: ‘write’ reading between 65536 and 2147483647 bytes from a region of size 16 [-Wstringop-overread]
 3468 |                 if (write(xc->writex_fd, s, len)) { };
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/build-via-sdist-b4bq_3__/pylibfst-0.2.1.dev0/fst/fstapi.c: In function ‘fstReaderIterBlocks2.part.0’:
/tmp/build-via-sdist-b4bq_3__/pylibfst-0.2.1.dev0/fst/fstapi.c:5899:46: note: source object ‘vcd_id’ of size 16
 5899 |                                         char vcd_id[16];
      |                                              ^~~~~~
In file included from /usr/include/zconf.h:492,
                 from /usr/include/zlib.h:34,
                 from /tmp/build-via-sdist-b4bq_3__/pylibfst-0.2.1.dev0/fst/fstapi.h:36,
                 from /tmp/build-via-sdist-b4bq_3__/pylibfst-0.2.1.dev0/fst/fstapi.c:45:
/usr/include/unistd.h:378:16: note: in a call to function ‘write’ declared with attribute ‘access (read_only, 2, 3)’
  378 | extern ssize_t write (int __fd, const void *__buf, size_t __n) __wur
      |                ^~~~~

This is due to the function fstVcdIDForFwrite which produces id strings of unbounded size. This function is applied, however, to arrays whose size is only 16. Two bytes of such an array are reserved for delimiters. Thus, the maximum allowable size of an id string is 14.

Execution

This pr explicitly limits the size of an id string to 14, thus removing the warning.

Remove deprecated function find_executable

Observation

In the build process when setup.py is run, the Python runtime complains about the use of the function distutils.find_executable which it flags as deprecated.

<string>:20: DeprecationWarning: Use shutil.which instead of find_executable

Execution

As suggested, this pr replaces distutils.find_executable with shutil.which. Imho, it would be advisable to ditch the dependency from distutils entirely.

Fix cmake minimum version

Observation

When using contemporary distributions of cmake a warning is issued that versions before 3.10 are deprecated. I have been using cmake version 3.31.1 which ships with Alpine Linux 3.21.0.

Execution

I have set the minimum version of cmake to 3.10.

@joergen7

joergen7 commented Jan 7, 2025

Copy link
Copy Markdown
Author

Hello, I realize that one of the changes I'm proposing touches an upstream repository: gtkwave. Do you want me to revert the changes to libfst and address these with the maintainers of gtkwave instead?

@joergen7

joergen7 commented Jan 7, 2025

Copy link
Copy Markdown
Author

The improvement of setup.py remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant