From: "Alejandro Colomar (man-pages)" <alx.manpages@gmail.com>
To: Jonny Grant <jg@jguk.org>
Cc: linux-man <linux-man@vger.kernel.org>,
Michael Kerrisk <mtk.manpages@gmail.com>,
Florian Weimer <fw@deneb.enyo.de>,
gcc-help@gcc.gnu.org
Subject: Re: strlen
Date: Thu, 8 Jul 2021 13:06:17 +0200 [thread overview]
Message-ID: <feb6c15d-b242-83fc-c58d-2ebfbcd4f2bd@gmail.com> (raw)
In-Reply-To: <5566b180-1333-d73b-22ee-6c6d32053921@jguk.org>
On 7/8/21 12:07 PM, Jonny Grant wrote:
> Thank you for your reply.
>
> We can't guarantee safestrlen() won't be called with NULL. So because strlen() itself doesn't check for NULL in C standard we'd need to call the wrapper so that NULL can be checked for.
>
> I'd like to avoid the compiler removing certain execution paths.
> I'd rather keep all code paths, even if they are not taken, just in case a NULL pointer creeps in due to an external device that is connected to an embedded system.
>
>
> Probably this would work:
>
> size_t __attribute__((optimize("O0"))) safestrlen(const char * s)
> {
> if (NULL == s) return 0;
> else return strlen(s);
> }
I don't think you don't need that. Unless there's a bug in GCC, it
shouldn't optimize that path unless it is 100% sure that it will never
be called.
Moreover, I recommend you to optimize as much as possible.
Even though NULL is possible in your code, I guess it's unlikely.
Also, calling a function safe is too generic.
I'd call it with the suffix null, as it act different on null.
Also, I recommend avoiding 'size_t' (and any other unsigned types, BTW).
See <https://google.github.io/styleguide/cppguide.html#Integer_Types>.
Use the POSIX type 'ssize_t'.
That also allows differentiating a length of 0 (i.e., "") from an
invalid string (i.e., NULL), by returning -1 for NULL.
Recommended implementation (requires C99 or later due to the usage of
inline):
[
// compiler.h
#define likely(x) __builtin_expect((x), 1)
#define unlikely(x) __builtin_expect((x), 0)
]
[
// strlennull.h
#include <string.h>
#include <sys/types.h>
#include "compiler.h"
[[gnu::pure]]
inline ssize_t strlennull(const char *s);
inline ssize_t strlennull(const char *s)
{
if (unlikely(!s))
return -1;
return strlen(s);
}
]
[
// strlennull.c
#include "strlennull.h"
#include <sys/types.h>
extern inline ssize_t strlennull(const char *s);
]
BTW, if the input is so untrusted that NULL may be possible, I guess it
might as well contain invalid strings (non-NUL-terminated), for which
strlen(3) would behave as bad or worse. So I recommend having a look at
strnlen(3) and maybe implement strnlennull() instead of strlennull().
Unless you _know_ there's a compiler bug that doesn't allow you to
optimize, please optimize. Otherwise, if it's just a bit of paranoia,
you could as well not optimize any code at all. Specifically not
optimizing this code by the use of pragmas or attributes is *wrong*.
Just revise the preprocessor output, the compiler output, and also try
introducing a NULL at run time, to check that everything's fine.
>
> I also use 'volatile' for reads/writes to addresses that I don't want to be optimized out.
Are you sure you don't want to optimize? Are those addresses hardware
I/O or interrupts?
Maybe you just want membarrier(2).
>
>>
>> What I recommend you to do from time to time, to make sure you don't miss any warnings, is compile the whole project first with '-O3' and then with '-O0'. If you are a bit paranoic, sporadically you can try all of them : '-Og', '-O0', '-O1', '-Os', '-O2', '-O3', '-Ofast' but I don't think that is necessary. Of course, always use '-fanalyzer' (GCC 10 and above).
>
> Yes, I am looking forward to David Malcom's -fanalyzer when Ubuntu LTS next upgrades, I'm on gcc 9.3 today. But -fanalyzer is only for C anyway.. much of of code base I work with is compiled as C++ so I can't use -fanalyzer yet.
You can install gcc-10 for Bionic: apt-get install gcc-10
I recommend it. It finds bugs very deep in the code.
Cheers,
Alex
--
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/
next parent reply other threads:[~2021-07-08 11:06 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <0bf239e9-cfc7-8889-ca00-b1d1566bd174@jguk.org>
[not found] ` <87lfhpgxf8.fsf@mid.deneb.enyo.de>
[not found] ` <017a5a66-ba66-7cc8-c607-f851c2e54fc4@jguk.org>
[not found] ` <87363whf2z.fsf@mid.deneb.enyo.de>
[not found] ` <48e874cb-2b95-2783-ddfc-ae12d9aaf8f5@jguk.org>
[not found] ` <87bl7fozxi.fsf@mid.deneb.enyo.de>
[not found] ` <23679a28-5986-0af2-4d98-7de68ed0deec@jguk.org>
[not found] ` <53b3666b-d200-ef97-b050-cc38669c61cd@gmail.com>
[not found] ` <b6fccca1-6e2b-fb20-d9d6-9df94cd3f05f@gmail.com>
[not found] ` <564825ed-1e1f-b344-da35-1b83c551ed5f@jguk.org>
[not found] ` <b71170df-7c6b-4582-c3d1-84b811fe5259@gmail.com>
[not found] ` <5566b180-1333-d73b-22ee-6c6d32053921@jguk.org>
2021-07-08 11:06 ` Alejandro Colomar (man-pages) [this message]
2021-07-08 12:05 ` strlen johnsfine
2021-07-09 2:08 ` strlen LIU Hao
2021-07-09 17:26 ` strlen Martin Sebor
2021-07-09 19:51 ` strlen johnsfine
2021-07-09 20:19 ` strlen Alejandro Colomar (man-pages)
2021-07-08 12:13 ` strlen Xi Ruoyao
2021-07-08 23:49 ` strlen Segher Boessenkool
2021-07-09 13:54 ` strlen Jonny Grant
2021-07-09 14:17 ` strlen Alejandro Colomar (man-pages)
2021-07-09 16:11 ` strlen Xi Ruoyao
2021-07-10 1:00 ` strlen Segher Boessenkool
2021-07-09 10:50 ` strlen Jonny Grant
2021-07-09 11:27 ` strlen Alejandro Colomar (man-pages)
2021-07-09 11:43 ` strlen Alejandro Colomar (man-pages)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=feb6c15d-b242-83fc-c58d-2ebfbcd4f2bd@gmail.com \
--to=alx.manpages@gmail.com \
--cc=fw@deneb.enyo.de \
--cc=gcc-help@gcc.gnu.org \
--cc=jg@jguk.org \
--cc=linux-man@vger.kernel.org \
--cc=mtk.manpages@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).