Quotes inside config values don't survive serialization/deserialization #758

Merged
merged 3 commits into from Jun 19, 2012

Conversation

Projects
None yet
7 participants
Contributor

aroben commented Jun 9, 2012

If you store a config value that contains quotes:

git_config_set_string(cfg, "user.name", "Dwayne \"The Rock\" Johnson");

then save the config to disk, load it again, and read the value back out:

git_config_get_string(&str, cfg, "user.name");

the interior quotes will be gone!

Note that if you read the value back out before saving to/loading from disk, the interior quotes are still there.

Contributor

aroben commented Jun 9, 2012

I'm hoping that someone who knows the config code better than I can tackle this one. But I've given you a test to start with!

This pull request passes (merged 0a3bec9f into e0b110e).

This pull request passes (merged f1b609ac into e0b110e).

Contributor

aroben commented Jun 9, 2012

Whaaaaat? The whole point is that it doesn't pass! And Travis shows that the build is red. What gives?

Owner

carlosmn commented Jun 9, 2012

Travis is wacko.

Have you checked what the file looks like when we write it out? I'm guessing that what we do is write the string as Dwayne "The Rock" Johnson so upon reading we discard the quotes.

Contributor

aroben commented Jun 9, 2012

Yes, that does seem to be what's happening. The quotes end up in the config file, but they are not escaped in any way.

Contributor

scottjg commented Jun 9, 2012

joshk commented Jun 9, 2012

I am sorry about the bot issues guys, we will look into this ASAP

Owner

vmg commented Jun 9, 2012

❤️ @joshk ❤️

joshk commented Jun 10, 2012

we have found the issue and a fix will be deployed tonight
sorry about the bugs guys
❤️❤️❤️

Contributor

aroben commented Jun 11, 2012

BTW:

$ git config foo.bar 'Dwayne "The Rock" Johnson'
$ git config foo.bar
Dwayne "The Rock" Johnson
[foo]
    bar = Dwayne \"The Rock\" Johnson

This pull request fails (merged 8c78c12 into e0b110e).

Contributor

aroben commented Jun 11, 2012

This pull request fails

Hooray!

This pull request fails (merged f00eb09 into e0b110e).

Contributor

aroben commented Jun 13, 2012

Is there anything left to be done here? Looks like Travis failed for an unrelated reason.

Owner

carlosmn commented Jun 13, 2012

The travis failure is related to timezone fixes that apparently MinGW doesn't like. This should be mergeable. @tanoku ?

Contributor

scottjg commented Jun 13, 2012

i think @benstraub fixed the mingw/timezone compilation issues already in 0284a21, anyway

@@ -237,12 +239,14 @@ static int config_set(git_config_file *cfg, const char *name, const char *value)
if (value) {
tmp = git__strdup(value);
GITERR_CHECK_ALLOC(tmp);
+ esc_value = escape_value(value);
@vmg

vmg Jun 13, 2012

Owner

Leaks As Fuck (TM)

src/config_file.c
}
git__free(existing->value);
existing->value = tmp;
- return config_write(b, existing->key, NULL, value);
+ return config_write(b, existing->key, NULL, esc_value);
@vmg

vmg Jun 13, 2012

Owner

Super Duper Leaker (C)2012

}
- if (config_write(b, key, NULL, value) < 0) {
+ if (config_write(b, key, NULL, esc_value) < 0) {
@vmg

vmg Jun 13, 2012

Owner

The Great Leak, Leakamount Pictures (R)

Owner

vmg commented Jun 13, 2012

...Yes, I'm not that helpful. But esc_value cannot be a const char*, needs to be char * and explicitly free'd.

Owner

carlosmn commented Jun 13, 2012

Sorry, I was sure I'd already handled the leaks. It's water-tight now.

src/config_file.c
+ return NULL;
+ }
+
+ fflush(stdout);
@aroben

aroben Jun 13, 2012

Contributor

Why is this needed? I don't see anywhere in this code that writes to stdout.

@vmg

vmg Jun 13, 2012

Owner

@carlosmn has the bad habit of littering the codebase with printf debug statements, and then forgetting to remove them. :)

@carlosmn

carlosmn Jun 13, 2012

Owner

😭

If you think the ones you see are bad, you should see how git diff looks like after I stage the relevant parts.

This pull request fails (merged f8b0c88a into 53774eb).

@@ -237,12 +238,17 @@ static int config_set(git_config_file *cfg, const char *name, const char *value)
if (value) {
@scunz

scunz Jun 13, 2012

Contributor

In case, value was given as NULL, this if leaves esc_value uninitialized, though it is used in config_write and git__free below

@carlosmn

carlosmn Jun 13, 2012

Owner

Yeah... I'd consider calling this function with NULL to be wrong enough that you deserve the library to crash on you :p Thanks for the catch, I'll fix it

@@ -256,13 +262,17 @@ static int config_set(git_config_file *cfg, const char *name, const char *value)
if (value) {
@scunz

scunz Jun 13, 2012

Contributor

Same here: In case, value was given as NULL, this if leaves esc_value uninitialized, though it is used in config_write and git__free below

This pull request fails (merged 22e69b23 into 53774eb).

This pull request passes (merged e4335ea0 into 53774eb).

carlosmn added some commits Jun 11, 2012

config: correctly escape quotes in the value
When a configuration option is set, we didn't check to see whether
there was any escaping needed. Escape the available characters so we
can unescape them correctly when we read them.

This pull request passes (merged 67d334c into 53774eb).

vmg added a commit that referenced this pull request Jun 19, 2012

Merge pull request #758 from libgit2/config-values-containing-quotes
Quotes inside config values don't survive serialization/deserialization

@vmg vmg merged commit 68f527c into development Jun 19, 2012

phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014

Merge pull request #758 from libgit2/config-values-containing-quotes
Quotes inside config values don't survive serialization/deserialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment