Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Fix warnings in output.c #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/output.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@
* Default is radians.
*/
static unsigned char sphere_output = OUTPUT_RAD;
static short int sphere_output_precision = DBL_DIG;

/*
* Defines the precision of floating point values in output.
*/
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
Contributor Author

@vitcpp vitcpp Aug 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 type int 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.


PG_FUNCTION_INFO_V1(set_sphere_output);
PG_FUNCTION_INFO_V1(spherepoint_out);
Expand Down