Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined behavior in ncx.c, nc_test #2796

Closed
WardF opened this issue Nov 9, 2023 · 6 comments
Closed

Undefined behavior in ncx.c, nc_test #2796

WardF opened this issue Nov 9, 2023 · 6 comments
Assignees
Milestone

Comments

@WardF
Copy link
Member

WardF commented Nov 9, 2023

We are currently seeing a failure in nc_test under the latest version of Visual Studio; the most recent update has resulted in this issue coming to light. Previously, it would run just fun. Currently, we only see the failure when running build type Release; build types Debug and RelWithDebInfo report success. The failure in this environment is 'value read not value expected' for CDF5 tests.

These failures are a red herring, and symptomatic, I believe, of a larger issue

In trying to debug this, I've compiled nc_test with -fsanitize=undefined using clang on MacOS, and then running nc_test. Output is attached, but as we can see, there are a number of instances where ncx.c relies on undefined behavior.

The issues are straight forward, but the sticking point is that ncx.c is generated from ncx.m4, and I am not an m4 wizard; I'm working through it to see what's going on and how to address some of these issues, but it is possible that somebody more familiar with m4 may see the solution faster. @DennisHeimbigner

Output from nc_test with -fsanitize=undefined attached.

For questions about how I compiled this, see the following for the high level details including what flags and options I disabled (I'm running this without DAP, NCZARR, filters, etc).

@WardF WardF added this to the 4.9.3 milestone Nov 9, 2023
@WardF
Copy link
Member Author

WardF commented Nov 9, 2023

Tagging @wkliao as a relevant expert/resource. Any insight is greatly appreciated :)

@WardF
Copy link
Member Author

WardF commented Nov 9, 2023

Follow up: Note that there are other files reporting Undefined behavior, but ncx.c is the one that corresponds, seemingly, to the original test failures.

@DennisHeimbigner
Copy link
Collaborator

I cannot duplicate this error. Is it possible to get github actions or appveyor to cause the error
to occur?

@WardF
Copy link
Member Author

WardF commented Nov 9, 2023

@DennisHeimbigner I do not expect it will be easy to recreate as it is the result of undefined behavior; I can only get it to manifest under specific circumstances locally.

Instead, on Linux, try this from the top level netcdf-c directory:

$ mkdir build && cd build
$ cmake .. -DCMAKE_C_FLAGS="-fsanitize=undefined" && cmake --build . --target nc_test -j 4 && ctest -V -R nc_test -E nc_test.+

You'll have to make adjustments for your local environment, library paths and whatnot, but you should see a whole screed of undefined behavior warnings.

Before I go too far trying to fix these, we have a backlog of PR's that I want to go through and merge; a few are related to eliminating warnings, so I am going to take a look at those first.

@wkliao
Copy link
Contributor

wkliao commented Dec 6, 2023

Does #2800 resolve this issue?

I do not see errors and I am using

CFLAGS="-fsanitize=undefined -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -g -Wall -Wconversion"

and gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

However, I do see the followings in ncdump test.

grep runtime */*.log
ncdump/tst_ncgen4.log:../../netcdf-c/ncgen/cvt.c:454:5: runtime error: null pointer passed as argument 2, which is declared to never be null
ncdump/tst_ncgen4.log:../../netcdf-c/ncgen/cvt.c:454:5: runtime error: null pointer passed as argument 2, which is declared to never be null
ncdump/tst_ncgen4.log:../../netcdf-c/ncgen/cvt.c:454:5: runtime error: null pointer passed as argument 2, which is declared to never be null

@WardF
Copy link
Member Author

WardF commented Dec 6, 2023

It appears to, yes @wkliao. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants