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

Possible mis-reading from using "strlen" on uninitialized string. #81

Open
cponder opened this issue May 11, 2022 · 8 comments
Open

Possible mis-reading from using "strlen" on uninitialized string. #81

cponder opened this issue May 11, 2022 · 8 comments

Comments

@cponder
Copy link
Contributor

cponder commented May 11, 2022

I'm testing MPAS-A with PNetCDF 1.12.3 and Valgrind is giving me messages like this:

1: ==2752438== Conditional jump or move depends on uninitialised value(s)
1: ==2752438==    at 0x4842658: strlen (vg_replace_strmem.c:494)
1: ==2752438==    by 0x4E2CB74: ncmpio_new_NC_var (ncmpio_var.c:71)
1: ==2752438==    by 0x4E26D8E: hdr_get_NC_var (ncmpio_header_get.c:1067)
1: ==2752438==    by 0x4E24201: ncmpio_hdr_get_NC (ncmpio_header_get.c:1239)

I get ~400 of these messages and am wondering if you can patch this on the PNetCDF side.

@cponder
Copy link
Contributor Author

cponder commented May 11, 2022

The variable name is declared and processed here in the file src/drivers/ncmpio/ncmpio_header_get.c

   1028     char *name;
   1029     NC_var *varp;
   1030 
   1031     /* get name */
   1032     err = hdr_get_NC_name(gbp, &name);
                           ....
   1066     /* allocate space for NC_var object */
   1067     varp = ncmpio_new_NC_var(name, ndims);

The function-call on line 1032 allocates the name here in the same file

    531     /* Allocate a NC_string structure large enough to hold nchars characters.
    532      * Note nchars is strlen(namestring) without terminal character.
    533      */
    534     *namep = (char*) NCI_Malloc((size_t)nchars + 1);
    535     if (*namep == NULL) DEBUG_RETURN_ERROR(NC_ENOMEM)
    536     (*namep)[nchars] = '\0'; /* add terminal character */

By my reading, the NCI_Malloc does not initialize the contents of the array.
In the file src/drivers/ncmpio/ncmpio_var.c the strlen gets invoked

     71     varp->name_len = strlen(name); /* name has been NULL checked */

which Valgrind is flagging.

@cponder
Copy link
Contributor Author

cponder commented May 12, 2022

It looks like what you're doing is measuring the length of the array, rather than the length of the string it contains, since it was never assigned a value along this execution-path.
You do append a '\0' to the end in the appropriate index to do this.
But you can't guarantee that there's not a random '\0' in one of the lower indices of the array, which would give an incorrect value.
In particular, if the array were initialized to zero, which is the most common case, this would match '\0' in the first position.

@cponder cponder changed the title Spurious "strlen" warning from Valgrind Possible mis-reading from using "strlen" on uninitialized string. May 12, 2022
@wkliao
Copy link
Member

wkliao commented May 12, 2022

The name buffer is overwritten in line 552.

memcpy(cpos, gbp->pos, (size_t)strcount);

Can you dump the header of the file?
Also, try ncvaidator with valgrind and see if the same warning appears.

@cponder
Copy link
Contributor Author

cponder commented May 12, 2022

I'm not sure what file is being read here. Can you give me a print-statement and line to insert it to?

@wkliao
Copy link
Member

wkliao commented May 12, 2022

The subroutines shown in valgrind message are called when reading a file.
If you don't know what file is being read, then you can add a printf statement
at the top of ncmpio_open() in file src/drivers/ncmpio/ncmpio_open.c

@wkliao
Copy link
Member

wkliao commented Jun 1, 2022

I made some changes to avoid using strlen as much as possible.
Could you give the master branch a try and see if valgrind still complains?

@cponder
Copy link
Contributor Author

cponder commented Jun 2, 2022

I can't figure out how to do the got clone inside my container-build.
If you could assign a tag to the current snapshot I'd be able to download the tarball.

@wkliao
Copy link
Member

wkliao commented Jun 2, 2022

Have you tried the following in your Dockerfile?

RUN git clone https://github.com/Parallel-NetCDF/PnetCDF.git && \
    cd PnetCDF && \
    autoreconf -i && \
    ./configure --prefix=/install/path && \
    make -j8 install

If that does not work, I can either send you a tar ball or create a tag for you.

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

No branches or pull requests

2 participants