On 2/8/08, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> If somebody wants to dip his or her toe in git hacking, and is
> tempted to send in a "clean up" patch (e.g. whitespace, coding
> style) that does not really _fix_ anything, please don't.
>
> I have a task of similar complexity (meaning, reasonably easy)
> that is much more useful and appreciated than clean-up patches
> for you.
>
> The callback functions that are passed to git_config() need to
> be audited so that they do not barf when given NULL. Currently,
> many of them are not safe.
>
> A callback function of git_config() is called when the command
> reads value from .git/config and friends. The function takes
> two parameters, var and value. var is never NULL and it is the
> name of the configuration variable found in the file being
> read. value could be either string or NULL.
>
> A NULL value is boolean "true". For example, on MS-DOS, you may
> have something like this:
>
> [core]
> autocrlf
>
> and your callback will be called with var = "core.autocrlf" and
> value = NULL in such a case.
>
> If you want to fix them (you do not have to do all of them, and
> if you would like to help, please make one patch per function
> fixed), the procedure is:
>
> (1) Find calling sites for git_config(). For example, we find
> one in archive-tar.c::write_tar_archive().
>
> int write_tar_archive(struct archiver_args *args)
> {
> int plen = args->base ? strlen(args->base) : 0;
>
> git_config(git_tar_config);
>
> archive_time = args->time;
> verbose = args->verbose;
> ...
>
> (2) Look at the function that is passed to git_config().
>
> static int git_tar_config(const char *var, const char *value)
> {
> if (!strcmp(var, "tar.umask")) {
> if (!strcmp(value, "user")) {
> tar_umask = umask(0);
> umask(tar_umask);
> } else {
> tar_umask = git_config_int(var, value);
> }
> return 0;
> }
> return git_default_config(var, value);
> }
>
> (3) Let's fix it. If the user's configuration has:
>
> [tar]
> umask
>
> it is an illegal configuration, but the code above does not
> check for NULL, and the second strcmp() would fail. If we
> guard that strcmp() with a check against NULL, we would be
> Ok. git_config_int() will correctly barf telling the user
> that "tar.umask" configuration is wrong.
>
> (4) Then send in a patch. Again, one patch per fixed function,
> please. The message may look like this:
>
> -- >8 --
> [PATCH] archive-tar.c: guard config parser from value=NULL
>
> Signed-off-by: A U Thor <author@xxxxxxxxxxx>
>
> archive-tar.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index e1bced5..30aa2e2 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -222,7 +222,7 @@ static void write_global_extended_header(const unsigned char *sha1)
> static int git_tar_config(const char *var, const char *value)
> {
> if (!strcmp(var, "tar.umask")) {
> - if (!strcmp(value, "user")) {
> + if (value && !strcmp(value, "user")) {
> tar_umask = umask(0);
> umask(tar_umask);
> } else {
> -
> 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
>
I can try my hand at that. I will send some patches later today (after work).
-Govind
-
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
