[PATCH 1/4] last: fix format overflow

[PATCH 1/4] last: fix format overflow

[Sami Kerola]

login-utils/last.c:624:23: warning: '%s' directive writing up to 31 bytes
into a region of size 27 [-Wformat-overflow=]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/last.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/login-utils/last.c b/login-utils/last.c
index 679ea6c12..153130ac0 100644
--- a/login-utils/last.c
+++ b/login-utils/last.c
@@ -598,7 +598,7 @@ static void __attribute__((__noreturn__)) usage(const struct last_control *ctl,
 static int is_phantom(const struct last_control *ctl, struct utmpx *ut)
 {
 	struct passwd *pw;
-	char path[32];
+	char path[sizeof("/dev/") + sizeof(ut->ut_line) + 1];
 	int ret = 0;
 
 	if (ut->ut_tv.tv_sec < ctl->boot_time.tv_sec)
-- 
2.13.0

[PATCH 2/4] libblkid: fix format overflow

[Sami Kerola]

libblkid/src/devname.c:166:29: warning: '%s' directive writing up to 255
bytes into a region of size 245 [-Wformat-overflow=]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libblkid/src/devname.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libblkid/src/devname.c b/libblkid/src/devname.c
index ba3c57a41..4ee6e3119 100644
--- a/libblkid/src/devname.c
+++ b/libblkid/src/devname.c
@@ -152,12 +152,13 @@ static int is_dm_leaf(const char *devname)
 {
 	struct dirent	*de, *d_de;
 	DIR		*dir, *d_dir;
-	char		path[256];
 	int		ret = 1;
 
 	if ((dir = opendir("/sys/block")) == NULL)
 		return 0;
 	while ((de = readdir(dir)) != NULL) {
+		char path[strlen(de->d_name) + sizeof("/sys/block//slaves") + 1];
+
 		if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..") ||
 		    !strcmp(de->d_name, devname) ||
 		    strncmp(de->d_name, "dm-", 3) ||
-- 
2.13.0

[PATCH 3/4] lib/loopdev: fix format overflow

[Sami Kerola]

lib/loopdev.c:546:32: warning: '/loop/backing_file' directive output may be
truncated writing 18 bytes into a region of size between 1 and 256
[-Wformat-truncation=]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/loopdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/loopdev.c b/lib/loopdev.c
index fd4f16692..c1d56f7b4 100644
--- a/lib/loopdev.c
+++ b/lib/loopdev.c
@@ -533,7 +533,7 @@ static int loopcxt_next_from_sysfs(struct loopdev_cxt *lc)
 	fd = dirfd(iter->sysblock);
 
 	while ((d = readdir(iter->sysblock))) {
-		char name[256];
+		char name[sizeof(d->d_name) + sizeof("/loop/backing_file") + 1];
 		struct stat st;
 
 		DBG(ITER, ul_debugobj(iter, "check %s", d->d_name));
-- 
2.13.0

[PATCH 4/4] lib/sysfs: fix format overflow

[Sami Kerola]

lib/sysfs.c:343:31: warning: '/start' directive output may be truncated
writing 6 bytes into a region of size between 1 and 256
[-Wformat-truncation=]

lib/sysfs.c:372:32: warning: '/partition' directive output may be truncated
writing 10 bytes into a region of size between 1 and 256
[-Wformat-truncation=]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/sysfs.c b/lib/sysfs.c
index cc290faac..6272b80b4 100644
--- a/lib/sysfs.c
+++ b/lib/sysfs.c
@@ -307,7 +307,7 @@ static struct dirent *xreaddir(DIR *dp)
 
 int sysfs_is_partition_dirent(DIR *dir, struct dirent *d, const char *parent_name)
 {
-	char path[256];
+	char path[strlen(d->d_name) + sizeof("/start") + 1];
 
 #ifdef _DIRENT_HAVE_D_TYPE
 	if (d->d_type != DT_DIR &&
@@ -356,7 +356,6 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
 {
 	DIR *dir;
 	struct dirent *d;
-	char path[256];
 	dev_t devno = 0;
 
 	dir = sysfs_opendir(cxt, NULL);
@@ -365,6 +364,7 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
 
 	while ((d = xreaddir(dir))) {
 		int n, maj, min;
+		char path[strlen(d->d_name) + sizeof("/partition") + 1];
 
 		if (!sysfs_is_partition_dirent(dir, d, NULL))
 			continue;
-- 
2.13.0

Re: [PATCH 4/4] lib/sysfs: fix format overflow

[Karel Zak]

On Sat, May 27, 2017 at 07:24:09PM +0100, Sami Kerola wrote:
> lib/sysfs.c:343:31: warning: '/start' directive output may be truncated
> writing 6 bytes into a region of size between 1 and 256
> [-Wformat-truncation=]
> 
> lib/sysfs.c:372:32: warning: '/partition' directive output may be truncated
> writing 10 bytes into a region of size between 1 and 256
> [-Wformat-truncation=]
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  lib/sysfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/sysfs.c b/lib/sysfs.c
> index cc290faac..6272b80b4 100644
> --- a/lib/sysfs.c
> +++ b/lib/sysfs.c
> @@ -307,7 +307,7 @@ static struct dirent *xreaddir(DIR *dp)
>  
>  int sysfs_is_partition_dirent(DIR *dir, struct dirent *d, const char *parent_name)
>  {
> -	char path[256];
> +	char path[strlen(d->d_name) + sizeof("/start") + 1];
>  
>  #ifdef _DIRENT_HAVE_D_TYPE
>  	if (d->d_type != DT_DIR &&
> @@ -356,7 +356,6 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
>  {
>  	DIR *dir;
>  	struct dirent *d;
> -	char path[256];
>  	dev_t devno = 0;
>  
>  	dir = sysfs_opendir(cxt, NULL);
> @@ -365,6 +364,7 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
>  
>  	while ((d = xreaddir(dir))) {
>  		int n, maj, min;
> +		char path[strlen(d->d_name) + sizeof("/partition") + 1];

why strlen() here? Maybe we can add to c.h macro 

 #define ul_dname_sizeof(x)
    (sizeof(((struct dirent *)0)->d_dname) + sizeof(x) + 1)

or use PATH_MAX ...

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 4/4] lib/sysfs: fix format overflow

[Sami Kerola]

On 29 May 2017 at 11:09, Karel Zak <kzak@redhat.com> wrote:
> On Sat, May 27, 2017 at 07:24:09PM +0100, Sami Kerola wrote:
>> lib/sysfs.c:343:31: warning: '/start' directive output may be truncated
>> writing 6 bytes into a region of size between 1 and 256
>> [-Wformat-truncation=]
>>
>> lib/sysfs.c:372:32: warning: '/partition' directive output may be truncated
>> writing 10 bytes into a region of size between 1 and 256
>> [-Wformat-truncation=]
>>
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>> ---
>>  lib/sysfs.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/sysfs.c b/lib/sysfs.c
>> index cc290faac..6272b80b4 100644
>> --- a/lib/sysfs.c
>> +++ b/lib/sysfs.c
>> @@ -307,7 +307,7 @@ static struct dirent *xreaddir(DIR *dp)
>>
>>  int sysfs_is_partition_dirent(DIR *dir, struct dirent *d, const char *parent_name)
>>  {
>> -     char path[256];
>> +     char path[strlen(d->d_name) + sizeof("/start") + 1];
>>
>>  #ifdef _DIRENT_HAVE_D_TYPE
>>       if (d->d_type != DT_DIR &&
>> @@ -356,7 +356,6 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
>>  {
>>       DIR *dir;
>>       struct dirent *d;
>> -     char path[256];
>>       dev_t devno = 0;
>>
>>       dir = sysfs_opendir(cxt, NULL);
>> @@ -365,6 +364,7 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
>>
>>       while ((d = xreaddir(dir))) {
>>               int n, maj, min;
>> +             char path[strlen(d->d_name) + sizeof("/partition") + 1];
>
> why strlen() here? Maybe we can add to c.h macro
>
>  #define ul_dname_sizeof(x)
>     (sizeof(((struct dirent *)0)->d_dname) + sizeof(x) + 1)
>
> or use PATH_MAX ...

For some reason I thought d_name would be allocated, in which case
strlen() would be
more appropriate. Thank you for pointing out it is very static 255
buffer, so added the
proposed ul_dname_sizeof() to c.h and changed the rest of the commits
accordingly.

https://github.com/kerolasa/lelux-utiliteetit/commit/5eb0ea80b376e295aca649acc8f8e2eaa91f30bb

Updates to changes are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git rc2-fixes

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 4/4] lib/sysfs: fix format overflow

[Yuriy M. Kaminskiy]

Sami Kerola <kerolasa@iki.fi> writes:

> On 29 May 2017 at 11:09, Karel Zak <kzak@redhat.com> wrote:
>> On Sat, May 27, 2017 at 07:24:09PM +0100, Sami Kerola wrote:
>>> lib/sysfs.c:343:31: warning: '/start' directive output may be truncated
>>> writing 6 bytes into a region of size between 1 and 256
>>> [-Wformat-truncation=]
>>>
>>> lib/sysfs.c:372:32: warning: '/partition' directive output may be truncated
>>> writing 10 bytes into a region of size between 1 and 256
>>> [-Wformat-truncation=]
>>>
>>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>>> ---
>>>  lib/sysfs.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/sysfs.c b/lib/sysfs.c
>>> index cc290faac..6272b80b4 100644
>>> --- a/lib/sysfs.c
>>> +++ b/lib/sysfs.c
>>> @@ -307,7 +307,7 @@ static struct dirent *xreaddir(DIR *dp)
>>>
>>>  int sysfs_is_partition_dirent(DIR *dir, struct dirent *d, const char *parent_name)
>>>  {
>>> -     char path[256];
>>> +     char path[strlen(d->d_name) + sizeof("/start") + 1];
>>>
>>>  #ifdef _DIRENT_HAVE_D_TYPE
>>>       if (d->d_type != DT_DIR &&
>>> @@ -356,7 +356,6 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
>>>  {
>>>       DIR *dir;
>>>       struct dirent *d;
>>> -     char path[256];
>>>       dev_t devno = 0;
>>>
>>>       dir = sysfs_opendir(cxt, NULL);
>>> @@ -365,6 +364,7 @@ dev_t sysfs_partno_to_devno(struct sysfs_cxt *cxt, int partno)
>>>
>>>       while ((d = xreaddir(dir))) {
>>>               int n, maj, min;
>>> +             char path[strlen(d->d_name) + sizeof("/partition") + 1];
>>
>> why strlen() here? Maybe we can add to c.h macro
>>
>>  #define ul_dname_sizeof(x)
>>     (sizeof(((struct dirent *)0)->d_dname) + sizeof(x) + 1)
>>
>> or use PATH_MAX ...
>
> For some reason I thought d_name would be allocated, in which case
> strlen() would be
> more appropriate. Thank you for pointing out it is very static 255
> buffer, so added the

Is this *defined by standard*? I believe not.
=== man dirent.h ===
       The character array d_name is of unspecified size, but  the  number  of
       bytes preceding the terminating null byte shall not exceed {NAME_MAX}.
=== cut ===
Same apply to [patch 3/4].

(Of course, both loopdev and sysfs are linux-specific, so
portablity is not very important (however, it is not that unheard of
from glibc or gcc folks to suddenly change those standard-unspecified
things, and then say "don't rely on undefined behavior, baka >_<"; then
again, there are uclibc, dietlibc, klibc, etc)).

> proposed ul_dname_sizeof() to c.h and changed the rest of the commits
> accordingly.
>
> https://github.com/kerolasa/lelux-utiliteetit/commit/5eb0ea80b376e295aca649acc8f8e2eaa91f30bb
>
> Updates to changes are available in the git repository at:
>   git://github.com/kerolasa/lelux-utiliteetit.git rc2-fixes

Re: [PATCH 4/4] lib/sysfs: fix format overflow

[Karel Zak]

On Wed, May 31, 2017 at 10:28:08PM +0300, Yuriy M. Kaminskiy wrote:
> > For some reason I thought d_name would be allocated, in which case
> > strlen() would be
> > more appropriate. Thank you for pointing out it is very static 255
> > buffer, so added the
> 
> Is this *defined by standard*? I believe not.
> === man dirent.h ===
>        The character array d_name is of unspecified size, but  the  number  of
>        bytes preceding the terminating null byte shall not exceed {NAME_MAX}.

I don't read this as sizeof(d->d_name) is wrong thing as it's array.
Anyway, use NAME_MAX seems like the most robust solution.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/4] last: fix format overflow

[Karel Zak]

On Sat, May 27, 2017 at 07:24:06PM +0100, Sami Kerola wrote:
> login-utils/last.c:624:23: warning: '%s' directive writing up to 31 bytes
> into a region of size 27 [-Wformat-overflow=]
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  login-utils/last.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/login-utils/last.c b/login-utils/last.c
> index 679ea6c12..153130ac0 100644
> --- a/login-utils/last.c
> +++ b/login-utils/last.c
> @@ -598,7 +598,7 @@ static void __attribute__((__noreturn__)) usage(const struct last_control *ctl,
>  static int is_phantom(const struct last_control *ctl, struct utmpx *ut)
>  {
>  	struct passwd *pw;
> -	char path[32];
> +	char path[sizeof("/dev/") + sizeof(ut->ut_line) + 1];

 I have applied patches from Rudi with NAME_MAX.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com