Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix warnings in output.c#51

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vitcpp merged 1 commit intopostgrespro:masterfromvitcpp:fix-warnings
Aug 14, 2023
Merged

Conversation

vitcpp
Copy link
Contributor

The variable sphere_output_precision type is changed to int because sprintf requires this argument to be of type int when used as the precision specificator (argument for '*' symbol in the sprintf template).

Copy link
Contributor

@esabolesabol left a comment

Choose a reason for hiding this comment

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

This looks fine. The 100 inbuf[100] should be more meticulously determined, but, if you’re happy with this, then let’s just merge and move on to incorporating PG16 support.

/*
* Define the precision of output floating point values.
*/
static int sphere_output_precision = DBL_DIG;
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don’t understand how changing this from short int to int fixes the compiler error:

src/output.c:424:18: error: ‘%.*g’ directive writing between 1 and 310 bytes into a region of size between 76 and 92 [-Werror=format-overflow=]  424 |       "%2ud %2um %.*gs",      |                  ^~~~src/output.c:424:7: note: assuming directive output of 309 bytes  424 |       "%2ud %2um %.*gs",      |       ^~~~~~~~~~~~~~~~~

but I do 100% agree that the argument to sprintf should be int. I’m going to just chalk this up to this gcc version not being quite right.

Copy link
ContributorAuthor

@vitcppvitcppAug 14, 2023
edited
Loading

Choose a reason for hiding this comment

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

@esabol sprintf takes variadic arguments that is implemented using va_list in C language. sprintf decides how to format the memory chunk with arguments based on the template. I guess, it is like to read raw bytes from a stream and try to interpret them. There is an undefined behaviour if sprintf expects int type (4 bytes), but short int (2 bytes) is passed instead. It seems 2 extra bytes belong to the next argument, depending on the implementation of va_list. It is a real bug that can lead to some unexpected program behaviour and it should be fixed.

The problem with some magic numbers in temporary buffers is not so important. The case with buffer overflow is unlikely in this case. If we want to redesign this behaviour it should be rewritten completely. Some checks for buffer overflows should be implemented.

P.S.
Man sprintf tells:

A field width, or precision, or both, may be indicated by an asterisk ( '*' ). In this case an argument of typeint supplies the field width or precision. Applications shall ensure that arguments specifying field width, or precision, or both appear in that order before the argument, if any, to be converted.

esabol reacted with thumbs up emoji
@vitcpp
Copy link
ContributorAuthor

@esabol It is interesting, that I tried this:

sprintf(&buf[0], "%.*gd, (int)sphere_output_precision, ...

And I found that it doesn't fix the warning on my side. I use gcc 11.3.0. The warning is enabled by -Wformat-overflow. Some heuristic in gcc is used in this case. It seems it somehow defines that sphere_output_precision may be much greater than DBL_DIG. I can't explain why such change doesn't fix the warning but change the type to int fixes it.

esabol reacted with thumbs up emoji

@vitcpp
Copy link
ContributorAuthor

vitcpp commentedAug 14, 2023
edited
Loading

The warnings are fixed for Focal dist. The rest will be fixed in PR#39. Lets incorporate this patch first,#39 will be the next.

esabol reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@esabolesabolesabol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@vitcpp@esabol

[8]ページ先頭

©2009-2025 Movatter.jp