-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
prefer 'vsnprintf' to 'vsprintf' #5561
Conversation
@guolinke @shiyu1994 could you please help with a review on this one? I think it's fairly small and noncontroversial, and well help LightGBM to maintain compatible with newer releases of the C/C++ toolchain on macOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Contributes to #5557.
A recent build of the
M1Mac
CRAN flavor included the following WARNING:This PR proposes replacing uses of
vsprintf()
in LightGBM withvsnprintf()
.Notes for Reviewers
Looking around GitHub, I can see that other projects have been experiencing this warning when upgrading to macOS Ventura + the newest version of XCode. https://github.com/search?q=%22Due+to+security+concerns+inherent+in+the+design+of+sprintf%22&type=issues. So I don't think this is specific to R or to LightGBM.
References
https://linux.die.net/man/3/vsnprintf describes the benefits of
vsnprintf()
. Basically, it is likevsprintf()
but requires that you provide an argumentsize_t size
, the maximum number of bytes that the function will write to the provided buffer. That added safety prevents use of this function to produce a buffer overflow.I checked, and
vsnprintf_s()
is also supported in all of the versions of MSVC LightGBM supports.