Bug 36

Summary: Inappropriate usage of strncpy
Product: Asunder Reporter: whz <pierre.lestringant+asunder>
Component: AllAssignee: Andrew Smith <asmith22+bugs>
Status: CONFIRMED ---    
Severity: enhancement CC: asmith22+bugs, pierre.lestringant+asunder
Priority: ---    
Version: 2.7   
Hardware: All   
OS: All   

Description whz 2017-03-25 10:12:33 EDT
File: util.c
Function: make_filename
strncpy(&ret[pos], path, strlen(path));
strncpy(&ret[pos], dir, strlen(dir));


I suggest to replace strncpy by strcpy. The length of these strings (path, dir, ...) has already been taken into account while allocating ret. No overflow is possible.
Comment 1 whz 2017-03-25 10:44:32 EDT
The same remark apply to:
- parse_format
- program_exists
Comment 2 whz 2017-03-25 10:59:20 EDT
grep strncpy *.c
The strncpy usage is inappropriate almost everywhere.
Comment 3 whz 2017-03-25 11:03:27 EDT
By the way, in get_default_prefs wouldn't it better to use strdup instead of all that complex stuff with malloc + strncpy?
Comment 4 Andrew Smith 2017-03-27 19:04:48 EDT

Thanks for the bug report. I feel that the use of strncpy is so unburdensome that I'd rather have the extra sanity check for the string operations than have to think about whether I'm going out of bounds every time I see strcpy in the code.

As to strdup() in get_default_prefs - yes it would work just as well and save a few lines of code. I'll take a patch if you care to send me one, but I don't feel the need to change it myself :)


Comment 5 whz 2017-03-28 03:12:44 EDT
The thing is that using strncpy this way does not bring any extra security at all. The third argument of strncpy is supposed to be the size of the destination buffer not the size of the source buffer.

strdup would help you remove the hardcoded string lengths, which in my opinion are quite a bad coding practice.
Comment 6 Andrew Smith 2017-03-28 12:19:27 EDT
What strncpy does is it allows me to very quickly check whether it is possible for that line of code to cause a buffer overflow. Which strcpys often do. If it were strcpy - I would have to do a lot more looking and thinking in order to establish the same.

I agree with you about strdup, feel free to fix it and attach a patch to this bug.