| Message ID | 20240910070910.18937-1-ceggers@arri.de |
|---|---|
| State | New |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | notifier: workaround for sscanf format mismatch | expand |
Hi Christian, On 10.09.24 09:09, Christian Eggers wrote: > ino_t is either 'unsigned long' or 'unsigned long long' (depending on > the platform and the C library version). As the C library doesn't > provide a printf/scanf 'conversion specifier' for ino_t, we shouldn't > pass a pointer of this type to sscanf. > > Signed-off-by: Christian Eggers <ceggers@arri.de> But is this not correctly handled by the inttypes.h header, that is included ? > --- > core/notifier.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/core/notifier.c b/core/notifier.c > index dace990951d1..3a762aa39335 100644 > --- a/core/notifier.c > +++ b/core/notifier.c > @@ -537,8 +537,8 @@ void notify_init(void) > */ > if (sd_booted() && getenv("JOURNAL_STREAM") != NULL) { > dev_t device; > - ino_t inode; > - if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%lu", &device, &inode) == 2) { > + unsigned long long inode; > + if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%llu", &device, &inode) == 2) { This patch tends to revert commit 64a9ab54, that was meant to fix these use cases. On 32bits systems, SCNu64 is %llu. Should this written as instead as if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%" SCNu64 ? > struct stat statbuffer; > if (fstat(fileno(stderr), &statbuffer) == 0) { > if (inode == statbuffer.st_ino && device == statbuffer.st_dev) { Best regards, Stefano Babic
Hi Stefano, On Tuesday, 10 September 2024, 11:40:16 CEST, Stefano Babic wrote: > Hi Christian, > > On 10.09.24 09:09, Christian Eggers wrote: > > ino_t is either 'unsigned long' or 'unsigned long long' (depending on > > the platform and the C library version). As the C library doesn't > > provide a printf/scanf 'conversion specifier' for ino_t, we shouldn't > > pass a pointer of this type to sscanf. > > > > Signed-off-by: Christian Eggers <ceggers@arri.de> > > But is this not correctly handled by the inttypes.h header, that is > included ? There is no "SCNino_t" in inttypes.h. I tried to create my own one depending on "#if sizeof(ino_t)==..." but using sizeof() in preprocessor is not possible. > > > --- > > core/notifier.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/core/notifier.c b/core/notifier.c > > index dace990951d1..3a762aa39335 100644 > > --- a/core/notifier.c > > +++ b/core/notifier.c > > @@ -537,8 +537,8 @@ void notify_init(void) > > */ > > if (sd_booted() && getenv("JOURNAL_STREAM") != NULL) { > > dev_t device; > > - ino_t inode; > > - if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%lu", &device, &inode) == 2) { > > + unsigned long long inode; > > + if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%llu", &device, &inode) == 2) { > > > > This patch tends to revert commit 64a9ab54, that was meant to fix these > use cases. On 32bits systems, SCNu64 is %llu. Should this written as > instead as > > if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%" SCNu64 ? I don't know whether there is a guarantee that ino_t is equal to uint64_t on all platforms. So I think it isn't a good idea to pass a "pointer to ino_t" directly to sscanf(). Instead I prefer using a temporary variable of a size which is >= sizeof(ino_t). For me it doesn't make much difference using either "unsigned long long" or "uint64_t" in such a case (as long there exists a related conversion specifier for this type). I would propose also not using "dev_t" directly with sscanf(). I overlooked this when I wrote the patch (as the compiler didn't complain). > > > struct stat statbuffer; > > if (fstat(fileno(stderr), &statbuffer) == 0) { > > if (inode == statbuffer.st_ino && device == statbuffer.st_dev) { > > Best regards, > Stefano Babic > regards, Christian
diff --git a/core/notifier.c b/core/notifier.c index dace990951d1..3a762aa39335 100644 --- a/core/notifier.c +++ b/core/notifier.c @@ -537,8 +537,8 @@ void notify_init(void) */ if (sd_booted() && getenv("JOURNAL_STREAM") != NULL) { dev_t device; - ino_t inode; - if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%lu", &device, &inode) == 2) { + unsigned long long inode; + if (sscanf(getenv("JOURNAL_STREAM"), "%" SCNu64 ":%llu", &device, &inode) == 2) { struct stat statbuffer; if (fstat(fileno(stderr), &statbuffer) == 0) { if (inode == statbuffer.st_ino && device == statbuffer.st_dev) {
ino_t is either 'unsigned long' or 'unsigned long long' (depending on the platform and the C library version). As the C library doesn't provide a printf/scanf 'conversion specifier' for ino_t, we shouldn't pass a pointer of this type to sscanf. Signed-off-by: Christian Eggers <ceggers@arri.de> --- core/notifier.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)