Skip to content

fix two unitialized variables found w/ valgrind#662

Merged
billsacks merged 1 commit into
ESCOMP:mainfrom
DeniseWorthen:bugfix/valgrindfixes
Jun 8, 2026
Merged

fix two unitialized variables found w/ valgrind#662
billsacks merged 1 commit into
ESCOMP:mainfrom
DeniseWorthen:bugfix/valgrindfixes

Conversation

@DeniseWorthen

Copy link
Copy Markdown
Collaborator

Description of changes

Initialize two character strings and correct typo.

Contributors other than yourself, if any: @DusanJovic-NOAA

Changes are B4B in UFS testing.

@billsacks billsacks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for these changes. I'm interested in hearing your thoughts on my comment below.

Comment thread mediator/esmFlds.F90
Comment on lines +48 to +49
character(CS) :: stdname = ''
character(CS) :: shortname = ''

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this change, but it makes me wonder if this might be masking a more serious issue. I don't have experience with valgrind, so I'm not sure: does the issue picked up in #661 suggest an actual attempt to reference a field with previously-uninitialized values, or just that valgrind can't guarantee that the data was initialized? If the former, I'm wondering if the real solution is to figure out the code path that has led to the use of a field with uninitialized stdname / shortname? Otherwise, it seems like we might be masking the problem by satisfying valgrind but with non-sensical data. @DeniseWorthen @DusanJovic-NOAA what do you think?

@DeniseWorthen DeniseWorthen Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial fix was more complicated (partly a result of querying AI for assistance). Those code changes were

diff --git a/mediator/esmFlds.F90 b/mediator/esmFlds.F90
index 7a2959f4..18b832b0 100644
--- a/mediator/esmFlds.F90
+++ b/mediator/esmFlds.F90
@@ -188,11 +188,17 @@ contains
     lastfld => fields
     found = .false.
     do while(associated(lastfld%next))
-       if (trim(stdname) == trim(lastfld%stdname)) exit
+       if (trim(stdname) == trim(lastfld%stdname)) then
+          found = .true.
+          exit
+       end if
        lastfld => lastfld%next
     enddo
+
     ! Check the lastfld
+    if (.not. found) then
        if (trim(stdname) == trim(lastfld%stdname)) found = .true.
+    end if

The attempt here was to ensure that if a match was not found, lastfld%next would still point to the very last (found) field. But Dusan reported that it did not resolve the issue.

I also don't have valgrind experience, but Dusan provided instructions and I can try to work through a better fix if you have ideas.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Claude's help, I think I understand the problem: When med_fldList_findName is called on a currently-empty list, the last line (if (trim(stdname) == trim(lastfld%stdname)) found = .true.) will be working on uninitialized data.

Now that I understand that issue, the fix here makes sense to me.

if (chkerr(rc,__LINE__,u_FILE_u)) return

if (merge_type /= 'unset' .and. merge_field /= 'unset') then
if (merge_type /= 'unset' .and. merge_fields /= 'unset') then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this change - thanks.

@billsacks billsacks merged commit 806cce1 into ESCOMP:main Jun 8, 2026
1 check passed
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.

uninitialized variables found by valgrind

2 participants