[MXS-253] Use of strncpy is dangerous Created: 2015-07-08  Updated: 2016-06-23  Resolved: 2016-06-23

Status: Closed
Project: MariaDB MaxScale
Component/s: N/A
Affects Version/s: 1.1.1, 1.2.0, 1.3.0, 1.4.0
Fix Version/s: 2.1.0

Type: Bug Priority: Major
Reporter: martin brampton (Inactive) Assignee: Johan Wikman
Resolution: Fixed Votes: 1
Labels: None
Environment:

All

Issue Links:
Blocks
is blocked by MXS-355 String handling Closed
Relates
relates to MXS-779 Review BLR strncpy usage. Closed
Epic Link: Refactoring
Sprint: 2016-12

 Description   

Use of strncpy is dangerous. For example, it is used in sharding_common.c at line 62 which reads:
strncpy(str,tok,MYSQL_DATABASE_MAXLEN);

If we have a cast iron guarantee that the string "tok" will not be longer than "MYSQL_DATABASE_MAXLEN" then the call is safe. But in that case, there is no advantage over using strcpy. If "tok" exceeded that length, then "str" will not have a terminating null, and results are unpredictable.

A safe way to use strncpy is:
strncpy(str1, str2, sizeof(str1)-1);
str1[sizeof(str1)-1] = '\0';

But it may be more sensible to check that the length of the source string is within the limit. At the very least finding a database name that exceeds what we think is the limit should be an error. Possibly it should cause MaxScale to crash on the grounds that once the situation falls outside the basic parameters that define the software, we don't know what may happen. Such a thing should never happen, but should be guarded against all the same.

This issue should not be cleared without checking all uses of strncpy (currently 99 total).

 Comments   
Comment by Massimiliano Pinto (Inactive) [ 2015-08-24 ]

strncpy work in progress in binlog router.

All buffers are initialised with all 0
char path[PATH_MAX+1] = "";
and usage is:
strncpy(path, inst->binlogdir, PATH_MAX);

Last buffer byte is 0 even for string size = PATH_MAX, given the above initialisation.

For string buffers passed to routines a comment will be added saying the var is already allocated by PATH_MAX+1

Comment by Johan Wikman [ 2015-09-05 ]

Significantly increased the estimate. This cannot be done as a mechanical operation, but the code must be carefully analysed for the intent before making modifications.

Comment by Johan Wikman [ 2015-09-07 ]

Lowering priority to minor.

The way strncpy is used in many places is clearly wrong, but given that this usage has been in MaxScale from the start, it is not likely to start causing crashes now. The buffers are most likely big enough so that the potential problem does not manifest itself.

This is better handled as as part of the general refactoring to be done post 1.3.

Comment by Johan Wikman [ 2016-02-29 ]

Moving to 1.5.

Comment by Johan Wikman [ 2016-06-23 ]

The strncpy usage in 

./core/gw_utils.c
 
./core/config.c
 
./core/dbusers.c
 
./core/maxpasswd.c
 
./core/secrets.c
 
./core/test/test_mysql_users.c
 
./core/service.c
 
./core/dcb.c
 
./core/resultset.c
 
./modules/protocol/mysql_backend.c
 
./modules/routing/maxinfo/maxinfo.c
 
./modules/routing/maxinfo/maxinfo_exec.c
 
./modules/routing/schemarouter/sharding_common.c
 
./modules/routing/schemarouter/schemarouter.c
 
./modules/routing/readwritesplit/readwritesplit.c
 
./modules/routing/avro/avro_client.c
 
./modules/routing/avro/avro_file.c
 
./modules/routing/avro/avro.c
 
./modules/filter/test/harness_ui.c
 
./modules/routing/binlog/binlog_common.c
 
./modules/authenticator/cdc_plain_auth.c
 
./modules/routing/binlog/maxbinlogcheck.c
 
./modules/routing/binlog/blr.c

has now been reviewed and updated where appropriate.

In BLR there is significant usage of strncpy and because of that, it will be handled separately in MXS-779.

Generated at Thu Feb 08 03:57:55 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.