Skip to content
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

CA-406953: fix more undefined behaviour #21

Merged
merged 8 commits into from
Mar 5, 2025

Conversation

edwintorok
Copy link
Contributor

When O_CREAT is used then the file mode must be specified, otherwise it'll be something random from the stack:

The mode argument must be supplied if O_CREAT or O_TMPFILE is specified in flags; if it is not supplied, some arbitrary bytes from the stack will be applied as the file mode

The file is opened read/write so set matching permission bits.

In command/stubs.c (used by writestatefile) we need to call vfprintf with ap, instead of fprintf.
fprintf would expect the actual arguments, whereas vfprintf will forward the varargs correctly.

Otherwise it warns that it is a non-literal and can't type check it.

Signed-off-by: Edwin Török <[email protected]>
The `v` was missing, and instead of calling the varargs version of printf, it called the regular one.

Signed-off-by: Edwin Török <[email protected]>
@edwintorok edwintorok force-pushed the private/edvint/undef2 branch 2 times, most recently from addf587 to 1c274ea Compare February 21, 2025 18:39
Copy link

@andyhhp andyhhp left a comment

Choose a reason for hiding this comment

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

Strictly speaking, the use of PMTC_S8 for format strings is undefined, because char has implementation-defined signed-ness. (Unlike other types in C, char and signed char do not mean the same thing.)

But, it's probably not worth boiling that particular ocean

Fixes:
```
/usr/include/x86_64-linux-gnu/bits/fcntl2.h:50:11: error: call to ‘__open_missing_mode’ declared with attribute error: open with O_CREAT or O_TMPFILE in second argument needs 3 arguments
```

Signed-off-by: Edwin Török <[email protected]>
@edwintorok edwintorok force-pushed the private/edvint/undef2 branch 2 times, most recently from d7ee2d6 to 1ca75ac Compare February 21, 2025 20:51
Do not rely on size of 'int' and 'long long', although they happened to work in this case.
Use stdint.h types instead.

Signed-off-by: Edwin Török <[email protected]>
Sometimes a format string with wrong signedness, or wrong size was used.
The wrong size is probably undefined behaviour.

Signed-off-by: Edwin Török <[email protected]>
@edwintorok edwintorok force-pushed the private/edvint/undef2 branch from 1ca75ac to 26a80ad Compare February 21, 2025 20:57
lib/weightio.c Outdated
@@ -121,7 +121,7 @@
MTC_STATUS
open_hostweight_file(int *fd, int *err_no)
{
if ((*fd = open(HA_HOST_WEIGHT_FILE, O_RDWR|O_CREAT)) < 0)
if ((*fd = open(HA_HOST_WEIGHT_FILE, O_RDWR|O_CREAT, 00400)) < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor, typo, one leading zero is enough.

@@ -322,7 +323,8 @@ MTC_STATIC void
print_liveset(
MTC_S32 pri,
PMTC_S8 log_string,
MTC_HOSTMAP hostmap);
MTC_HOSTMAP hostmap)
__attribute__((format(printf, 2, 0)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure? I cannot see the parameters, how can be a formatting string? Maybe it's a bug?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, never mind, the 0 says there are no parameter to check against. Maybe a kind of trick can be in place like

static inline const char *
check_print_liveset_format(const char *fmt, ...) __attribute__((format(printf, 1, 2))
{
    return fmt;
}

#define print_liveset(pri, fmt, map) print_liveset(pri, check_print_liveset_format(fmt, "dummy"), map)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler is still not entirely happy about it, because it says that the 'log_string' is not a string literal, whereas in the other function where I used format(printf,2,3) it was able to see that fmt is a string literal from the caller and didn't warn about it again.

I'll try the macro trick, that should fix it.

@GeraldEV GeraldEV merged commit 93b821c into xenserver:master Mar 5, 2025
4 checks 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.

5 participants