Skip to content

Conversation

@ax3l
Copy link
Member

@ax3l ax3l commented Mar 16, 2022

Spotted buffer overflows in sprintf with GCC 10.3.

  • fix me

X-ref: ignores AMReX-Codes/amrex#2750 for now (can be a separate PR)

@etpalmer63
Copy link
Contributor

Some quick grepping leads to

Dataset.cpp:  char maxInfoV[Amrvis::LINELENGTH],  minInfoV[Amrvis::LINELENGTH];

Which leads to:

./amrex/Src/Extern/amrdata/AMReX_AmrvisConstants.H:const int LINELENGTH = 160;

I guess we cannot just change LINELENGTH because it affects how AMRDATA behaves. (see amrex/Extern/amrdata/AMReX_AmrData.cpp. I will ask wiser people.

@ax3l
Copy link
Member Author

ax3l commented Mar 17, 2022

Jup, it's basically writing a string added to a prefix in a same-size string a couple of time.

I would say we trash the C pointer logic and make it C++? :)

@WeiqunZhang
Copy link
Member

Yes, there are quite some C strings and sprintf's that should be replaced with C++ strings.

ax3l added 2 commits March 17, 2022 10:37
Spotted buffer overflows in `sprintf` with GCC 10.3
@ax3l ax3l force-pushed the fix-Wformat-overflow branch from 6f677d5 to c3c74b4 Compare March 17, 2022 17:41
Fix string buffer overflows in two files.
@ax3l
Copy link
Member Author

ax3l commented Mar 17, 2022

@WeiqunZhang can you please take a look at AMReX-Codes/amrex#2660 ? :)
That one I do not immediately get, the other things I fixed now in Amrvis.

sprintf(minInfoV, fstring, rMin);
sprintf(minInfo, "Min:%s", minInfoV);
XmString sNewMin = XmStringCreateSimple(minInfo);
std::string minInfo("Min:");
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change this for compatibility, but we could add a space here:

Suggested change
std::string minInfo("Min:");
std::string minInfo("Min: ");

sprintf(maxInfoV, fstring, rMax);
sprintf(maxInfo, "Max:%s", maxInfoV);
XmString sNewMax = XmStringCreateSimple(maxInfo);
std::string maxInfo("Max:");
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change this for compatibility, but we could add a space here:

Suggested change
std::string maxInfo("Max:");
std::string maxInfo("Max: ");

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.

3 participants