12692: Use of untrusted string value 12687..12691: Unbounded source buffer
git patch file (master) -- 12692 - Use of untrusted string value 0001-Ticket-47835-Coverity-12687.12692.patch
12691 - Unbounded source buffer 0002-Ticket-47835-Coverity-12687.12692.patch
12690 - Unbounded source buffer 0003-Ticket-47835-Coverity-12687.12692.patch
12689 - Unbounded source buffer 0004-Ticket-47835-Coverity-12687.12692.patch
12688 - Unbounded source buffer 0005-Ticket-47835-Coverity-12687.12692.patch
12687 - Unbounded source buffer 0006-Ticket-47835-Coverity-12687.12692.patch
Reviewed by Rich (Thank you!!)
Pushed to master: b6b7199..43c6ff2 master -> master commit 43c6ff2e7801ff6bbc03961b3161dd60aebf707a commit 0a546bcb3d4625d6db1dcbb342922b4ddb3bee37 commit 48f2ea020bca3ee750885a6e5f423fae5e3606ca commit 162604a620ba75f2a5eed3095930a2aaa823a645 commit f25c7f1f988783d620171f7b648f946dc6704c81 commit 8dc3806d75b6e3d4722047e230db68ac20ab3e69
I would like to ask what is wrong with calling strdup. It should be safe. I saw many replacement of strdup in patches. In my opinion, it is a false positive.
For example: {{{ diff --git a/ldap/servers/slapd/tools/dbscan.c b/ldap/servers/slapd/tools/dbscan.c index 023fade..bbfcd0e 100644 --- a/ldap/servers/slapd/tools/dbscan.c +++ b/ldap/servers/slapd/tools/dbscan.c @@ -1077,16 +1077,17 @@ is_changelog(char *filename)
static void usage(char argv0) { - char copy = strdup(argv0); + long arg_max = sysconf(_SC_ARG_MAX); + char copy = strndup(argv0, arg_max); }}} Variable ''argv0'' has type ''char ''. So it is nul terminated string.
The manual page says: {{{ The strdup() function returns a pointer to a new string which is a duplicate of the string s. Memory for the new string is obtained with malloc(3), and can be freed with free(3). }}}
I cannot see a reason why there should be any problem. Could you explain me it? I would like to learn something new.
Yes, it could be false positive. I'm just worried the case when the given string is corrupted/tainted (e.g., no NULL terminated).
We had this code for years and have not heard of any security issue. But we thought it was a good practice to set the maximum length to the string to duplicate not to overflow anything... At least, you see no harm on that, don't you?
I'm not sure if a string passed from an environment variable, or from a command line argument, can really be non-NULL terminated. We should do some research on that, and also possibly find out what Coverity users usually do to fix such issues.
I cannot say it was a complete research :p, but it was based upon the search result on the coverity error and fix... I don't mind undoing the changes and just mark "false positive" on coverity if that's better.
One line fix: 13ec1d6..6e175e3 master -> master commit 6e175e3308f04de528254871f6b554fe992dd3df Description: arg_max has to be always set. But introduced by commit 0a546bcb3d4625d6db1dcbb342922b4ddb3bee37
It turned out these defects are false positive. Reverting... commit 43c6ff2e7801ff6bbc03961b3161dd60aebf707a commit 0a546bcb3d4625d6db1dcbb342922b4ddb3bee37 commit 48f2ea020bca3ee750885a6e5f423fae5e3606ca commit 162604a620ba75f2a5eed3095930a2aaa823a645 commit f25c7f1f988783d620171f7b648f946dc6704c81 commit 8dc3806d75b6e3d4722047e230db68ac20ab3e69 commit 6e175e3308f04de528254871f6b554fe992dd3df
Reverted: daf4b42..3a66ec7 master -> master {{{ commit 3a66ec7ca61ee273eb016fe234a3da8202e58e83 This reverts commit 43c6ff2e7801ff6bbc03961b3161dd60aebf707a. commit dc4527b6f6ebe7a2c42eb57942af535c31c5cd59 This reverts commit 0a546bcb3d4625d6db1dcbb342922b4ddb3bee37. commit f5014300dcdf46d9e432e1a8cee640595b862d6f This reverts commit 48f2ea020bca3ee750885a6e5f423fae5e3606ca. commit 50654969bcfeb0aa379c10ceda3cfd72993a527c This reverts commit 162604a620ba75f2a5eed3095930a2aaa823a645. commit cef82090cc3ece1a72c2ec1b62205b1f68b1074f This reverts commit f25c7f1f988783d620171f7b648f946dc6704c81. commit 8247976f25c22799a31be08074cc150e07f5dcce This reverts commit 8dc3806d75b6e3d4722047e230db68ac20ab3e69. commit 4b66c032996c7a313202f315683aa75e83d5f361 This reverts commit 6e175e3308f04de528254871f6b554fe992dd3df. }}}
Metadata Update from @nhosoi: - Issue set to the milestone: 1.3.3 backlog
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1166
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Invalid)