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

ksmbd-tools: handle unset ret #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ksmbd-tools: handle unset ret #323

wants to merge 1 commit into from

Conversation

neheb
Copy link

@neheb neheb commented Jan 9, 2025

If g_mapped_file_get_length returns 0 or SIZE_MAX, the loop does not get executed and ret does not get set

Move the assignment out of the loop and change to while loop.

Found with GCC's -fanalyzer.

If g_mapped_file_get_length returns 0 or SIZE_MAX, the loop does not get
executed and ret does not get set

Move the assignment out of the loop and change to while loop.

Found with GCC's -fanalyzer.

Signed-off-by: Rosen Penev <[email protected]>
@neheb
Copy link
Author

neheb commented Jan 9, 2025

I assume CI fails now because ubuntu-latest was recently updated.

len = g_mapped_file_get_length(file);
if (len == 0 || len == SIZE_MAX) {
ret = -EINVAL;
goto out_close;
Copy link
Member

Choose a reason for hiding this comment

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

g_mapped_file_unref() is not called on error handling ?. So, should we use goto out_unref instead of out_close ?

Copy link
Author

Choose a reason for hiding this comment

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

ah yes you're right. copy/paste mistake looks like.

@atheik
Copy link
Member

atheik commented Jan 9, 2025

@neheb,

The purpose of the (size_t)-1 check here is not to check if g_mapped_file_get_length() returned it.
Instead, its purpose is to handle situations where there is no trailing newline at the end of the file.
With your changes, the handling of such files breaks:

==8003== Invalid read of size 1
==8003==    at 0x484F239: memchr (vg_replace_strmem.c:986)
==8003==    by 0x111407: __mmap_parse_file (config_parser.c:251)
==8003==    by 0x1122BD: cp_parse_smbconf (config_parser.c:691)
==8003==    by 0x113FE4: load_config (tools.c:295)
==8003==    by 0x11CD64: worker_init (mountd.c:259)
==8003==    by 0x11CD64: manager_init (mountd.c:396)
==8003==    by 0x11D91F: mountd_main (mountd.c:456)
==8003==    by 0x4A26E07: (below main) (libc_start_call_main.h:58)
==8003==  Address 0x485c000 is not stack'd, malloc'd or (recently) free'd

Note that g_mapped_file_get_length() does not return 0 for us, since the preceding g_mapped_file_get_contents() returns NULL for empty files. If you would like to handle g_mapped_file_get_length() actually returning (size_t)-1 (and also satisfy the static analyzer), then please do this instead and remove the other changes:

diff --git a/tools/config_parser.c b/tools/config_parser.c
index b890b96..0040061 100644
--- a/tools/config_parser.c
+++ b/tools/config_parser.c
@@ -238,7 +238,7 @@ static int __mmap_parse_file(const char *path, process_entry_fn *process_entry)
 		goto out_unref;
 	}
 
-	for (len = g_mapped_file_get_length(file);
+	for (ret = -EINVAL, len = g_mapped_file_get_length(file);
 	     len > 0 && len != (size_t)-1;
 	     len -= delim - contents + 1, contents = delim + 1) {
 		g_autofree char *entry = NULL;

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.

3 participants