On Saturday 28 March 2009, Junio C Hamano wrote: > Johan Herland <johan@xxxxxxxxxxx> writes: > > - if (chmod(filename, 0444) || adjust_shared_perm(filename)) > > + if (chmod(filename, get_shared_perm(0444))) > > Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would > most likely say "no" and cause real harm, but there is no guarantee that > we won't start checking S_ISREG() or other things in get_shared_perm() > later. I do not like this. You are right. > How about doing it this way instead? > > One thing to note is that we seem to have been passing what we read from > st.st_mode, together with S_IFMT bits, to chmod(2); I do not think I've > seen any breakage reports on exotic systems (glibc on Linux seems to > ignore the higher bits), but from my reading of POSIX, I would not be > surprised if somebody's chmod(2) returned EINVAL. Agreed. > -- >8 -- > set_shared_perm(): sometimes we know what the final mode bits should look > like > > adjust_shared_perm() first obtains the mode bits from lstat(2), expecting > to find what the result of applying user's umask is, and then tweaked it s/tweaked/tweaks/ > as necessary. When the file to be adjusted is created with mkstemp(3), > however, the mode thusly obtained does not have anything to do with > usre's umask, and we would need to start from 0444 in such a case and s/usre/user/ > there is no point running lstat(2) for such a path. > > This introduces a new API set_shared_perm() to bypass the lstat(2) and > instead force setting the mode bits to the desired value directly. > adjust_shared_perm() becomes a thin wrapper to the function. > > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > --- [...] > diff --git a/sha1_file.c b/sha1_file.c > index 8869488..5bfc36c 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const > char *filename) * > * The same holds for FAT formatted media. > * > - * When this succeeds, we just return 0. We have nothing > + * When this succeeds, we just return; we have nothing Small nit: This belongs in the previous patch, doesn't it? All in all, this looks very good. Please drop my second patch, and use this instead. Have fun! :) ...Johan -- Johan Herland, <johan@xxxxxxxxxxx> www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
