fixed format-truncation warning

[vjay82]

Fixed a warning which prevents building in Arch-Linux. Read somewhere that these warnings became errors from a specific GCC version on. This is what happens without the fix:

CC criu/plugin.o criu/plugin.c: In function 'cr_plugin_init': criu/plugin.c:246:36: error: '%s' directive output may be truncated writing between 3 and 2147483645 bytes into a region of size 4095 [-Werror=format-truncation=] 246 | snprintf(path, sizeof(path), "%s/%s", opts.libdir, de->d_name); | ^~ criu/plugin.c:246:3: note: 'snprintf' output 5 or more bytes (assuming 2147483647) into a destination of size 4096 246 | snprintf(path, sizeof(path), "%s/%s", opts.libdir, de->d_name); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[2]: *** [/tmp/criu/scripts/nmk/scripts/build.mk:119: criu/plugin.o] Error 1 make[1]: *** [criu/Makefile:76: criu/built-in.o] Error 2 make: *** [Makefile:259: criu] Error 2

[rst0git]

Hi @vjay82, thank you for fixing this. Could you please add a Signed-off-by line to your commit message?

git commit --amend -s

[vjay82]

@rst0git Hi, done.

[Snorch]

The initial warning was about buffer truncation is not checked. So I think the proper fix would be:

diff --git a/criu/plugin.c b/criu/plugin.c
index b97d3763a..67e7806d8 100644
--- a/criu/plugin.c
+++ b/criu/plugin.c
@@ -243,7 +243,9 @@ int cr_plugin_init(int stage)
                if (len < 3 || strncmp(de->d_name + len - 3, ".so", 3))
                        continue;
 
-               snprintf(path, sizeof(path), "%s/%s", opts.libdir, de->d_name);
+               if (snprintf(path, sizeof(path), "%s/%s", opts.libdir,
+                            de->d_name) >= sizeof(path))
+                       goto err;
 
                if (cr_lib_load(stage, path))
                        goto err;

Also you should add the initial warn message to the patch.

[vjay82]

@Snorch You are right, one should check both <0 && >= size => error.

[Snorch]

You are right, one should check both <0 && >= size => error.

@vjay82 No. To fix warning we need only the latter. And I think <0 check would be excess.

Though we have ~5(from total 326) places where we check snprintf return <0 and according to manual snprintf can return -1. But I think that in general case of valid pointers and proper size provided this function would not fail, only truncation is possible here.

[avagin]

Fixed a warning which prevents building in Arch-Linux

I tried to build CRIU in Arch-linux and don't see any errors: https://github.com/checkpoint-restore/criu/pull/1225

[vjay82]

Hello avagin,

maybe it only happens on the ARMv7 version, where I tried it? Anyway, it is a warning, which could be fixed.

Quoting from your linked page: "Thus, a return value of size or more means that the output was truncated." and "If an output error is encountered, a negative value is returned."

Example: space 10 -> sizeof = 10 wants to write 11, returns 11 (excluding the additional \0, which would have been written, see your link), which matches the (length_needed>=sizeof) -> goto error

Example: space 10 -> sizeof = 10 wants to write 10, returns 10 (because that \0 would not have fit), which also matches the (length_needed>=sizeof) -> goto error

For the rest of your comment: I can rewrite it to use the len variable if you like but I am not proficient enough in this project to do adequate error logging or anything like that - feel free to close my pull-request and do a better implementation.

Also, I am not very eager to discuss if a <0 check could be shaved off to save a byte or two. I can relate to the desire to create perfect solutions but again, I am not proficient enough to have a good/strong opinion. If you think, this could improve the project and is necessary, ok, close the request and do your own fix (or don't and confront others with that problem) but next implemented GCC warning could be about exactly that and breaking the build again in the future. So I would tend to catch everything that could go wrong. That branch predictors that bring us so much trouble these days make the cost go away anyway.

If sb. had only invented exceptions ... :D

[Snorch]

Probably we can add general macro to utils:

#define xsnprintf(str, size, fmt, ...)						\
	({									\
		int ___ret = snprintf(str, size, fmt, __VA_ARGS__);		\
		if (___ret < 0)							\
			pr_perror("%s: Failed snprintf %m", __func__);		\
		else if (___ret >= size)					\
			 pr_err("%s: Failed snprintf overflow", __func__);	\
		___ret;								\
	})

and when just s/snprintf/xsnprintf/ everywhere?

upd: we have ssprintf in tests for example.

[github-actions[bot]]

A friendly reminder that this PR had no activity for 30 days.