[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

[Lennart Poettering]

On Mon, 16.03.15 20:33, Goffredo Baroncelli (kreijack at libero.it) wrote:

Sorry for warming up this old thread, but a few issues I see with this?

>          RECURSIVE_RELABEL_PATH = 'Z',
> +        SET_ATTRIB = 'h',
> +        RECURSIVE_SET_ATTRIB = 'H',
>  } ItemType;
>  
>  typedef struct Item {
> @@ -108,12 +111,15 @@ typedef struct Item {
>          usec_t age;
>  
>          dev_t major_minor;
> +        int attrib_value;
> +        int attrib_mask;

I thought this was unsigned now?

>  
> +static int get_attrib_from_arg(Item *item) {
> +        static const unsigned attributes[] = {
> +                [(uint8_t)'A'] = FS_NOATIME_FL,    /* do not update atime */
> +                [(uint8_t)'S'] = FS_SYNC_FL,      /* Synchronous updates */
> +                [(uint8_t)'D'] = FS_DIRSYNC_FL,   /* dirsync behaviour (directories only) */
> +                [(uint8_t)'a'] = FS_APPEND_FL,    /* writes to file may only append */
> +                [(uint8_t)'c'] = FS_COMPR_FL,     /* Compress file */
> +                [(uint8_t)'d'] = FS_NODUMP_FL,    /* do not dump file */
> +                [(uint8_t)'e'] = FS_EXTENT_FL,    /* Top of directory hierarchies*/
> +                [(uint8_t)'i'] = FS_IMMUTABLE_FL, /* Immutable file */
> +                [(uint8_t)'j'] = FS_JOURNAL_DATA_FL, /* Reserved for ext3 */
> +                [(uint8_t)'s'] = FS_SECRM_FL,     /* Secure deletion */
> +                [(uint8_t)'u'] = FS_UNRM_FL,      /* Undelete */
> +                [(uint8_t)'t'] = FS_NOTAIL_FL,    /* file tail should not be merged */
> +                [(uint8_t)'T'] = FS_TOPDIR_FL,    /* Top of directory hierarchies*/
> +                [(uint8_t)'C'] = FS_NOCOW_FL,     /* Do not cow file */
> +        };

Hmm, that's quite a sparse array! It wastes 116 entries, just to
store 13 entries... I think this should really be an array fo structs
with the chars and their flag, which we iterate through... 

> +        const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
> +                FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
> +                FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
> +                FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
> +        char *p = item->argument;
> +        enum {
> +                MODE_ADD,
> +                MODE_DEL,
> +                MODE_SET
> +        } mode = MODE_ADD;
> +        unsigned long value = 0, mask = 0;

And now it is "unsigned long", not just "unsigned anymore?

> +
> +static int path_set_attrib(Item *item, const char *path) {
> +        _cleanup_close_ int fd = -1;
> +        int r;
> +        unsigned f;
> +        struct stat st;
> +
> +        /* do nothing */
> +        if (item->attrib_mask == 0 || !item->attrib_set)
> +                return 0;
> +        /*
> +         * It is OK to ignore an lstat() error, because the error
> +         * will be catch by the open() below anyway
> +         */
> +        if (lstat(path, &st) == 0 &&
> +            !S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) {
> +                return 0;
> +        }

Why do we need this check? What happens if we apply this onto
a device node, or socket, or whatever? 

I really don#t like this seperate lstat() + open(). If it all it
should be open() + fstat(), to avoid TTOCTOU issues...

> +        fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
> +
> +        if (fd < 0)
> +                return log_error_errno(errno, "Cannot open \"%s\": %m", path);
>  
> +        case SET_ATTRIB:
> +        case RECURSIVE_SET_ATTRIB:
> +                if (!i.argument) {
> +                        log_error("[%s:%u] Set attrib requires
> argument.", fname, line);

Please don't abbreviate words like "attrib" in human language...

Lennart

-- 
Lennart Poettering, Red Hat