Skip to content
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

Missing unsigned char libc interface semantics #6

Open
sortie opened this issue Jan 12, 2015 · 8 comments
Open

Missing unsigned char libc interface semantics #6

sortie opened this issue Jan 12, 2015 · 8 comments

Comments

@sortie
Copy link

sortie commented Jan 12, 2015

If you have these interfaces, you should audit them for whether they use unsigned char semantics (rather than signed char) as required by the standard: strcasecmp, memset, strncmp, memrchr, strchrnul, strcmp, stresep, strrchr, memchr, strncasecmp, memccpy, memcmp, fputc family, fgetc family, btowc, and wctob. Note also the <ctype.h> functions such as isspace has the domain of EOF and values representable as unsigned char, anything else is undefined behavior.

For instance, the sort order in strcmp is wrong.

@thepowersgang
Copy link
Owner

Audited and fixed list (commit 7d1c355)

  • memset
  • strcmp / strncmp
  • strcasecmp / strncasecmp
  • memchr
  • memcmp
  • fgetc / fputc

Absent:

  • strchrnul - GNU Extension
  • strsep - BSDisim
  • memccpy - POSIX, may implement in posix library eventually
  • btowc and wctob - No wide string support

@sortie
Copy link
Author

sortie commented Jan 12, 2015

I commented a bunch on the commit, found a bunch of issues. These kind of bugs will give weird third party software failures in the future. I can't comment on strncpy it seems, not part of the commit, but it has a bug where it accesses src before checking num. For instance, strncpy(dest, NULL, 0) might dereference NULL. Additonally, it doesn't zero out the rest of the buffer. Yeah, this strncpy implementation is entirely wrong. strncpy copies a string into a fixed width buffer, and if the input string is long enough it doesn't get nul-terminated, and if it's too short, it gets padded with nuls. It's an awful interface. Consider adding strlcpy and strlcat if you don't have them.

@sortie
Copy link
Author

sortie commented Jan 12, 2015

strncat is also wrong for that matter.

@sortie
Copy link
Author

sortie commented Jan 12, 2015

strstr looks wrong, it doesn't backtrack after a failed partial match. It's possible to do this correctly in O(N) time efficiently, might want to look up knuth morris pratt or what it's called.

@sortie
Copy link
Author

sortie commented Jan 12, 2015

I suggest adding abort or something to memcpy in case of overlapping copies, at least ind ebug builds, it'll help catch undefined behavior cases.

@sortie
Copy link
Author

sortie commented Jan 12, 2015

strndup doesn't check for malloc failure.

@sortie
Copy link
Author

sortie commented Jan 12, 2015

strtok_r is suspicious. The use of strchr there is very inefficient. I suggest you use strspn.

@ghost
Copy link

ghost commented Aug 23, 2015

I think this should be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants