Bug 36 - Inappropriate usage of strncpy
Summary: Inappropriate usage of strncpy
Status: CONFIRMED
Alias: None
Product: Asunder
Classification: Unclassified
Component: All (show other bugs)
Version: 2.7
Hardware: All All
: enhancement
Assignee: Andrew Smith
URL:
Depends on:
Blocks:
 
Reported: 2017-03-25 10:12 EDT by whz
Modified: 2017-03-28 12:19 EDT (History)
2 users (show)

See Also:


Attachments

Description whz 2017-03-25 10:12:33 EDT
File: util.c
Function: make_filename
Example:
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
Hi

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 :)

Cheers,

Andrew
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.

Andrew

Note You need to log in before you can comment on or make changes to this bug.