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

String handling fixes #19

Merged
merged 6 commits into from Jul 29, 2015

Conversation

Projects
None yet
2 participants
@tuomasjjrasanen
Contributor

tuomasjjrasanen commented Jul 28, 2015

Hi

This pull request contains various string handling fixes, mainly some off-by-one buffer overflow fixes and string termination fixes.

tuomasjjrasanen added some commits Jul 24, 2015

Fix off-by-one buffer overflow in config_read_file()
The maximum field width specifier of the format string for sscanf() does *not*
include the terminating NUL character which means that if the size of the
destination buffer equals the maximum field width, sscanf() will write the
terminating NUL character outside the buffer if a maxmimum sized field was
parsed.
Ignore garbage in config files
sscanf() might return 0 if an early matching failure is encountered. In this
case it means a line which has an invalid config option name.

Previously this error condition did not get checked which resulted in unexpected
parsing behavior: the previous valid configuration option got handled twice
because config_handle_option() was called with previously filled name array.

For example, consider the following config file:

channel_upper = 48
  =
channel = 1

It's obvious that the second line is malformed. Previously it caused
channel_upper config option to be handled twice with following trace:

Using config file 'horst.conf'
Set 'channel_upper' = '48'
Set 'channel_upper'

(And then horst segfaulted because conf_channel_upper() expects to get non-NULL
 string as its parameter.)

This commit fixes the issue simply by ignoring and logging all malformed config
lines.
Fix off-by-one buffer overflow in control_send_command()
The command is always terminated with NEWLINE + NUL, so the buffer needs to be
two characters longer than the string length. Previously, NUL was written past
the end of the buffer.
Fix off-by-one buffer overflow of node name arrays
Buffer overflow is fixed by allocating one character bigger array (for NUL
terminator) for the node name.

MAX_NODE_NAME_LEN is also renamed to MAX_NODE_NAME_STRLEN to emphasize its
meaning.
Fix off-by-one buffer overflows in string conf options
Buffer overflows are fixed by allocating one char bigger arrays (for NUL
terminator) for all string conf options.

MAX_CONF_VALUE_LEN is also renamed to MAX_CONF_VALUE_STRLEN to emphasize its
meaning.
Ensure strncpy() results are NUL-terminated
strncpy() itself does not ensure that the destination string is NUL-terminated:
if first N characters of the source string does include NUL, the destination
string won't be NUL-terminated. This commit fixes all call sites of strncpy() to
overwrite the last char with NUL.
@br101

This comment has been minimized.

Show comment
Hide comment
@br101

br101 Jul 28, 2015

Owner

I wonder if we should write a strncpy replacement and use it instead, and keep the string declarations without the LEN+1... Now it's a bit inconsistent...

Owner

br101 commented Jul 28, 2015

I wonder if we should write a strncpy replacement and use it instead, and keep the string declarations without the LEN+1... Now it's a bit inconsistent...

@tuomasjjrasanen

This comment has been minimized.

Show comment
Hide comment
@tuomasjjrasanen

tuomasjjrasanen Jul 28, 2015

Contributor

Yeah, I considered creating a strncpy() wrapper and using it instead, but I wasn't sure what you would have thought about that, so I just fixed the actual issue. A wrapper function/macro for strncpy() + last char to NUL can be implemented on top of those fixes, but not the other way around.

Moreover, libbsd has strlcpy() which fixes strncpy()'s issues. So if adding libbsd to dependencies is ok, I would go with that.

About the inconsistency, I'm not 100% sure what you consider inconsistent here, I think all real string declarations are now of form:

char mystringvar[MY_STRING_VAR_STRLEN + 1];

And then there are some other array declarations of type char, which are not strings (strings are arrays of chars terminated by NUL), for example mac addresses which are just arrays of 6 bytes.

Buf of course string declarations could be fixed by incrementing the size constant by one, then all declarations would of form:

#define MAX_CONF_VALUE_SIZE 201
char dumpfile[MAX_CONF_VALUE_SIZE];
Contributor

tuomasjjrasanen commented Jul 28, 2015

Yeah, I considered creating a strncpy() wrapper and using it instead, but I wasn't sure what you would have thought about that, so I just fixed the actual issue. A wrapper function/macro for strncpy() + last char to NUL can be implemented on top of those fixes, but not the other way around.

Moreover, libbsd has strlcpy() which fixes strncpy()'s issues. So if adding libbsd to dependencies is ok, I would go with that.

About the inconsistency, I'm not 100% sure what you consider inconsistent here, I think all real string declarations are now of form:

char mystringvar[MY_STRING_VAR_STRLEN + 1];

And then there are some other array declarations of type char, which are not strings (strings are arrays of chars terminated by NUL), for example mac addresses which are just arrays of 6 bytes.

Buf of course string declarations could be fixed by incrementing the size constant by one, then all declarations would of form:

#define MAX_CONF_VALUE_SIZE 201
char dumpfile[MAX_CONF_VALUE_SIZE];

br101 added a commit that referenced this pull request Jul 29, 2015

@br101 br101 merged commit 4db6f41 into br101:master Jul 29, 2015

@br101

This comment has been minimized.

Show comment
Hide comment
@br101

br101 Jul 29, 2015

Owner

I want to avoid another dependency on libbsd, and you are right, I can add a wrapper after your changes. Thanks!

Owner

br101 commented Jul 29, 2015

I want to avoid another dependency on libbsd, and you are right, I can add a wrapper after your changes. Thanks!

@tuomasjjrasanen

This comment has been minimized.

Show comment
Hide comment
@tuomasjjrasanen

tuomasjjrasanen Jul 29, 2015

Contributor

Yep, probably a wise decision, the most essential part of strlcpy()'s semantics is actually just this (copied from strncpy()s manpage):

strncpy(buf, str, buflen - 1);
    if (buflen > 0)
        buf[buflen - 1]= '\0';

Of course there are then some minor details about the return value, but those can be ignored in our case.

Contributor

tuomasjjrasanen commented Jul 29, 2015

Yep, probably a wise decision, the most essential part of strlcpy()'s semantics is actually just this (copied from strncpy()s manpage):

strncpy(buf, str, buflen - 1);
    if (buflen > 0)
        buf[buflen - 1]= '\0';

Of course there are then some minor details about the return value, but those can be ignored in our case.

@tuomasjjrasanen tuomasjjrasanen deleted the tuomasjjrasanen:string_handling_fixes branch Jul 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment