| Message ID | f7031316a043b36fac10ecf784d2294894967e7b.1708212896.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9abe31f5f161be4d69118bdfae00103cd6efa510 |
| Headers | show |
| Series | osxkeychain: bring in line with other credential helpers | expand |
On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget <gitgitgadget@gmail.com> wrote: > The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago. > The replacement SecItem API however is available as far back as macOS > 10.6. > > While supporting older macOS was perhaps prevously a concern, > git-credential-osxkeychain already requires a minimum of macOS 10.7 > since 5747c8072b (contrib/credential: avoid fixed-size buffer in > osxkeychain, 2023-05-01) so using the newer API should not regress the > range of macOS versions supported. > > Adapting to use the newer SecItem API also happens to fix two test > failures in osxkeychain: > > 8 - helper (osxkeychain) overwrites on store > 9 - helper (osxkeychain) can forget host > > The new API is compatible with credentials saved with the older API. > > Signed-off-by: Bo Anderson <mail@boanderson.me> I haven't studied the SecItem API, so I can't comment on the meat of the patch, but I can make a few generic observations... > diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c > @@ -3,14 +3,39 @@ > -__attribute__((format (printf, 1, 2))) > +#define ENCODING kCFStringEncodingUTF8 > +static CFStringRef protocol; /* Stores constant strings - not memory managed */ > +static CFStringRef host; > [...] > + > +static void clear_credential(void) > +{ > + if (host) { > + CFRelease(host); > + host = NULL; > + } > + [...] > +} > + > +__attribute__((format (printf, 1, 2), __noreturn__)) The addition of `__noreturn__` to the `__attribute__` seems unrelated to the stated purpose of this patch. As such, it typically would be placed in its own patch. If it really is too minor for a separate patch, mentioning it in the commit message as a "While at it..." would be helpful. > @@ -19,70 +44,135 @@ static void die(const char *err, ...) > +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...) > +{ > + va_list args; > + const void *key; > + CFMutableDictionaryRef result; > + > + result = CFDictionaryCreateMutable(allocator, > + 0, > + &kCFTypeDictionaryKeyCallBacks, > + &kCFTypeDictionaryValueCallBacks); > + > + Style: one blank line is preferred over two > + va_start(args, allocator); > + while ((key = va_arg(args, const void *)) != NULL) { > + const void *value; > + value = va_arg(args, const void *); > + if (value) > + CFDictionarySetValue(result, key, value); > + } > + va_end(args); A couple related comments... If va_arg() ever returns NULL for `value`, the next iteration of the loop will call va_arg() again, but calling va_arg() again after it has already returned NULL is likely undefined behavior. At minimum, I would have expected this to be written as: while (...) { ... if (!value) break; CFDictionarySetValue(...); } However, isn't it a programmer error if va_arg() returns NULL for `value`? If so, I'd think we'd want to scream loudly about that rather than silently ignoring it. So, perhaps something like this: while (...) { ... if (!value) { fprintf(stderr, "BUG: ..."); abort(); } CFDictionarySetValue(...); } Or, perhaps just call the existing die() function in this file with a suitable "BUG ..." message. > +static void find_username_in_item(CFDictionaryRef item) > { > + CFStringRef account_ref; > + char *username_buf; > + CFIndex buffer_len; > > + account_ref = CFDictionaryGetValue(item, kSecAttrAccount); > + if (!account_ref) > + { > + write_item("username", "", 0); > + return; > + } Style: opening brace sticks to the `if` line: if !(account_ref) { ... } Same comment applies to the `if` below. > + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING); > + if (username_buf) > + { > + write_item("username", username_buf, strlen(username_buf)); > return; > + } According to the documentation for CFStringGetCStringPtr(), the returned C-string is not newly-allocated, so the caller does not have to free it. Therefore, can `username_buf` be declared `const char *` rather than `char *` to make it clear to readers that nothing is being leaked here? Same comment about the `(char *)` cast. > + /* If we can't get a CString pointer then > + * we need to allocate our own buffer */ Style: /* * Multi-line comments * are formatted like this. */ > + buffer_len = CFStringGetMaximumSizeForEncoding( > + CFStringGetLength(account_ref), ENCODING) + 1; > + username_buf = xmalloc(buffer_len); > + if (CFStringGetCString(account_ref, > + username_buf, > + buffer_len, > + ENCODING)) { > + write_item("username", username_buf, buffer_len - 1); > + } > + free(username_buf); Okay, this explains why `username_buf` is declared `char *` rather than `const char *`. Typically, when we have a situation in which a value may or may not need freeing, we use a `to_free` variable like this: const char *username_buf; char *to_free = NULL; ... username_buf = (const char *)CFStringGetCStringPtr(...); if (username_buf) { ... return; } ... username_buf = to_free = xmalloc(buffer_len); if (CFStringGetCString(...)) ... free(to_free); But that may be overkill for this simple case, and what you have here may be "good enough" for anyone already familiar with the API and who knows that the `return` after CFStringGetCStringPtr() isn't leaking. > +static OSStatus find_internet_password(void) > { > + CFDictionaryRef attrs; > + [...] > > + attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne, > + kSecReturnAttributes, kCFBooleanTrue, > + kSecReturnData, kCFBooleanTrue, > + NULL); > + result = SecItemCopyMatching(attrs, (CFTypeRef *)&item); > + if (result) { > + goto out; > + } We omit braces when the body is a single statement: if (result) goto out; (Same comment applies to other code in this patch.) > + data = CFDictionaryGetValue(item, kSecValueData); > + [...] > + > +out: > + CFRelease(attrs); Good, `attrs` is released in all cases. > +static OSStatus add_internet_password(void) > { > + [...] > + attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password, > + NULL); > + result = SecItemAdd(attrs, NULL); > + if (result == errSecDuplicateItem) { > + CFDictionaryRef query; > + query = CREATE_SEC_ATTRIBUTES(NULL); > + result = SecItemUpdate(query, attrs); > + CFRelease(query); > + } > + CFRelease(attrs); > + return result; > } Good, `attrs` and `query` are released in all cases.
> On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote: > > I haven't studied the SecItem API, so I can't comment on the meat of > the patch, but I can make a few generic observations... Thanks for taking a look! >> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c >> @@ -3,14 +3,39 @@ >> -__attribute__((format (printf, 1, 2))) >> +#define ENCODING kCFStringEncodingUTF8 >> +static CFStringRef protocol; /* Stores constant strings - not memory managed */ >> +static CFStringRef host; >> [...] >> + >> +static void clear_credential(void) >> +{ >> + if (host) { >> + CFRelease(host); >> + host = NULL; >> + } >> + [...] >> +} >> + >> +__attribute__((format (printf, 1, 2), __noreturn__)) > > The addition of `__noreturn__` to the `__attribute__` seems unrelated > to the stated purpose of this patch. As such, it typically would be > placed in its own patch. If it really is too minor for a separate > patch, mentioning it in the commit message as a "While at it..." would > be helpful. Acknowledged. It is indeed a bit of a nothing change that doesn’t really do much on its own, but when paired with the port variable reorder could potentially make a “minor code cleanup” commit. >> + va_start(args, allocator); >> + while ((key = va_arg(args, const void *)) != NULL) { >> + const void *value; >> + value = va_arg(args, const void *); >> + if (value) >> + CFDictionarySetValue(result, key, value); >> + } >> + va_end(args); > > A couple related comments... > > If va_arg() ever returns NULL for `value`, the next iteration of the > loop will call va_arg() again, but calling va_arg() again after it has > already returned NULL is likely undefined behavior. At minimum, I > would have expected this to be written as: > > while (...) { > ... > if (!value) > break; > CFDictionarySetValue(...); > } > > However, isn't it a programmer error if va_arg() returns NULL for > `value`? If so, I'd think we'd want to scream loudly about that rather > than silently ignoring it. So, perhaps something like this: > > while (...) { > ... > if (!value) { > fprintf(stderr, "BUG: ..."); > abort(); > } > CFDictionarySetValue(...); > } > > Or, perhaps just call the existing die() function in this file with a > suitable "BUG ..." message. > In this case it’s by design to accept and check for NULL values as it greatly simplifies the code. Inputs to the credential helpers have various optional fields, such as port and path. It is programmer error to pass NULL to the SecItem API (runtime crash) so in order to simplify having to check each individual field in all of the callers (and probably ditch varargs since you can’t really do dynamic varargs), I check the value here instead. That means you can do something like: create_dictionary(kCFAllocatorDefault, kSecAttrServer, host, kSecAttrPath, path, \ kSecAttrPort, port, NULL) And it will only include the key-value pairs that have non-NULL values. It would indeed be programmer error to not pass key-value pairs, though it is equally programmer error to not have a terminating NULL. >> + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING); >> + if (username_buf) >> + { >> + write_item("username", username_buf, strlen(username_buf)); >> return; >> + } > > According to the documentation for CFStringGetCStringPtr(), the > returned C-string is not newly-allocated, so the caller does not have > to free it. Therefore, can `username_buf` be declared `const char *` > rather than `char *` to make it clear to readers that nothing is being > leaked here? Same comment about the `(char *)` cast. > >> + /* If we can't get a CString pointer then >> + * we need to allocate our own buffer */ > > Style: > > /* > * Multi-line comments > * are formatted like this. > */ > >> + buffer_len = CFStringGetMaximumSizeForEncoding( >> + CFStringGetLength(account_ref), ENCODING) + 1; >> + username_buf = xmalloc(buffer_len); >> + if (CFStringGetCString(account_ref, >> + username_buf, >> + buffer_len, >> + ENCODING)) { >> + write_item("username", username_buf, buffer_len - 1); >> + } >> + free(username_buf); > > Okay, this explains why `username_buf` is declared `char *` rather > than `const char *`. Typically, when we have a situation in which a > value may or may not need freeing, we use a `to_free` variable like > this: > > const char *username_buf; > char *to_free = NULL; > ... > username_buf = (const char *)CFStringGetCStringPtr(...); > if (username_buf) { > ... > return; > } > ... > username_buf = to_free = xmalloc(buffer_len); > if (CFStringGetCString(...)) > ... > free(to_free); > > But that may be overkill for this simple case, and what you have here > may be "good enough" for anyone already familiar with the API and who > knows that the `return` after CFStringGetCStringPtr() isn't leaking. Would it make sense to just have a comment paired with the CFStringGetCStringPtr return explaining why it doesn’t need to be freed there? I’m OK with the to_free variable however if that’s clearer. Idea in my mind was pairing it based on `xmalloc` but I can see why pairing based on variable is clearer.
On Sun, Feb 18, 2024 at 9:48 AM Bo Anderson <mail@boanderson.me> wrote: > > On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote: > >> + va_start(args, allocator); > >> + while ((key = va_arg(args, const void *)) != NULL) { > >> + const void *value; > >> + value = va_arg(args, const void *); > >> + if (value) > >> + CFDictionarySetValue(result, key, value); > >> + } > >> + va_end(args); > > > > However, isn't it a programmer error if va_arg() returns NULL for > > `value`? If so, I'd think we'd want to scream loudly about that rather > > than silently ignoring it. So, perhaps something like this: [...] > > In this case it’s by design to accept and check for NULL values as > it greatly simplifies the code. Inputs to the credential helpers > have various optional fields, such as port and path. It is > programmer error to pass NULL to the SecItem API (runtime crash) so > in order to simplify having to check each individual field in all of > the callers (and probably ditch varargs since you can’t really do > dynamic varargs), I check the value here instead. That means you can > do something like: > > create_dictionary(kCFAllocatorDefault, > kSecAttrServer, host, > kSecAttrPath, path, \ > kSecAttrPort, port, > NULL) > > And it will only include the key-value pairs that have non-NULL > values. > > It would indeed be programmer error to not pass key-value pairs, > though it is equally programmer error to not have a terminating > NULL. Okay. I had thought that this check was merely protecting against programmer error, but the described use-case to avoid passing NULL to SecItem API makes perfect sense. It might be helpful to future readers to explain this either as a function-level comment (explaining how to call the function) or as an in-code comment. > >> + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING); > >> + if (username_buf) > >> + { > >> + write_item("username", username_buf, strlen(username_buf)); > >> return; > >> + } > > > > According to the documentation for CFStringGetCStringPtr(), the > > returned C-string is not newly-allocated, so the caller does not have > > to free it. Therefore, can `username_buf` be declared `const char *` > > rather than `char *` to make it clear to readers that nothing is being > > leaked here? Same comment about the `(char *)` cast. > > > >> + buffer_len = CFStringGetMaximumSizeForEncoding( > >> + CFStringGetLength(account_ref), ENCODING) + 1; > >> + username_buf = xmalloc(buffer_len); > >> + if (CFStringGetCString(account_ref, > >> + username_buf, > >> + buffer_len, > >> + ENCODING)) { > >> + write_item("username", username_buf, buffer_len - 1); > >> + } > >> + free(username_buf); > > > > Okay, this explains why `username_buf` is declared `char *` rather > > than `const char *`. Typically, when we have a situation in which a > > value may or may not need freeing, we use a `to_free` variable like > > this: [...] > > > > But that may be overkill for this simple case, and what you have here > > may be "good enough" for anyone already familiar with the API and who > > knows that the `return` after CFStringGetCStringPtr() isn't leaking. > > Would it make sense to just have a comment paired with the > CFStringGetCStringPtr return explaining why it doesn’t need to be > freed there? I’m OK with the to_free variable however if that’s > clearer. Idea in my mind was pairing it based on `xmalloc` but I can > see why pairing based on variable is clearer. Most likely, anyone working on this code is already familiar with the CoreFoundation API, thus would understand implicitly that this isn't leaking. But, yes, a simple comment should be plenty sufficient for everyone else if you are re-rolling anyhow.
diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile index 4b3a08a2bac..238f5f8c36f 100644 --- a/contrib/credential/osxkeychain/Makefile +++ b/contrib/credential/osxkeychain/Makefile @@ -8,7 +8,8 @@ CFLAGS = -g -O2 -Wall -include ../../../config.mak git-credential-osxkeychain: git-credential-osxkeychain.o - $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -Wl,-framework -Wl,Security + $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \ + -framework Security -framework CoreFoundation git-credential-osxkeychain.o: git-credential-osxkeychain.c $(CC) -c $(CFLAGS) $< diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 5f2e5f16c88..dc294ae944a 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -3,14 +3,39 @@ #include <stdlib.h> #include <Security/Security.h> -static SecProtocolType protocol; -static char *host; -static char *path; -static char *username; -static char *password; -static UInt16 port; - -__attribute__((format (printf, 1, 2))) +#define ENCODING kCFStringEncodingUTF8 +static CFStringRef protocol; /* Stores constant strings - not memory managed */ +static CFStringRef host; +static CFStringRef path; +static CFStringRef username; +static CFDataRef password; +static CFNumberRef port; + +static void clear_credential(void) +{ + if (host) { + CFRelease(host); + host = NULL; + } + if (path) { + CFRelease(path); + path = NULL; + } + if (username) { + CFRelease(username); + username = NULL; + } + if (password) { + CFRelease(password); + password = NULL; + } + if (port) { + CFRelease(port); + port = NULL; + } +} + +__attribute__((format (printf, 1, 2), __noreturn__)) static void die(const char *err, ...) { char msg[4096]; @@ -19,70 +44,135 @@ static void die(const char *err, ...) vsnprintf(msg, sizeof(msg), err, params); fprintf(stderr, "%s\n", msg); va_end(params); + clear_credential(); exit(1); } -static void *xstrdup(const char *s1) +static void *xmalloc(size_t len) { - void *ret = strdup(s1); + void *ret = malloc(len); if (!ret) die("Out of memory"); return ret; } -#define KEYCHAIN_ITEM(x) (x ? strlen(x) : 0), x -#define KEYCHAIN_ARGS \ - NULL, /* default keychain */ \ - KEYCHAIN_ITEM(host), \ - 0, NULL, /* account domain */ \ - KEYCHAIN_ITEM(username), \ - KEYCHAIN_ITEM(path), \ - port, \ - protocol, \ - kSecAuthenticationTypeDefault - -static void write_item(const char *what, const char *buf, int len) +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...) +{ + va_list args; + const void *key; + CFMutableDictionaryRef result; + + result = CFDictionaryCreateMutable(allocator, + 0, + &kCFTypeDictionaryKeyCallBacks, + &kCFTypeDictionaryValueCallBacks); + + + va_start(args, allocator); + while ((key = va_arg(args, const void *)) != NULL) { + const void *value; + value = va_arg(args, const void *); + if (value) + CFDictionarySetValue(result, key, value); + } + va_end(args); + + return result; +} + +#define CREATE_SEC_ATTRIBUTES(...) \ + create_dictionary(kCFAllocatorDefault, \ + kSecClass, kSecClassInternetPassword, \ + kSecAttrServer, host, \ + kSecAttrAccount, username, \ + kSecAttrPath, path, \ + kSecAttrPort, port, \ + kSecAttrProtocol, protocol, \ + kSecAttrAuthenticationType, \ + kSecAttrAuthenticationTypeDefault, \ + __VA_ARGS__); + +static void write_item(const char *what, const char *buf, size_t len) { printf("%s=", what); fwrite(buf, 1, len, stdout); putchar('\n'); } -static void find_username_in_item(SecKeychainItemRef item) +static void find_username_in_item(CFDictionaryRef item) { - SecKeychainAttributeList list; - SecKeychainAttribute attr; + CFStringRef account_ref; + char *username_buf; + CFIndex buffer_len; - list.count = 1; - list.attr = &attr; - attr.tag = kSecAccountItemAttr; + account_ref = CFDictionaryGetValue(item, kSecAttrAccount); + if (!account_ref) + { + write_item("username", "", 0); + return; + } - if (SecKeychainItemCopyContent(item, NULL, &list, NULL, NULL)) + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING); + if (username_buf) + { + write_item("username", username_buf, strlen(username_buf)); return; + } - write_item("username", attr.data, attr.length); - SecKeychainItemFreeContent(&list, NULL); + /* If we can't get a CString pointer then + * we need to allocate our own buffer */ + buffer_len = CFStringGetMaximumSizeForEncoding( + CFStringGetLength(account_ref), ENCODING) + 1; + username_buf = xmalloc(buffer_len); + if (CFStringGetCString(account_ref, + username_buf, + buffer_len, + ENCODING)) { + write_item("username", username_buf, buffer_len - 1); + } + free(username_buf); } -static void find_internet_password(void) +static OSStatus find_internet_password(void) { - void *buf; - UInt32 len; - SecKeychainItemRef item; + CFDictionaryRef attrs; + CFDictionaryRef item; + CFDataRef data; + OSStatus result; - if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item)) - return; + attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne, + kSecReturnAttributes, kCFBooleanTrue, + kSecReturnData, kCFBooleanTrue, + NULL); + result = SecItemCopyMatching(attrs, (CFTypeRef *)&item); + if (result) { + goto out; + } - write_item("password", buf, len); + data = CFDictionaryGetValue(item, kSecValueData); + + write_item("password", + (const char *)CFDataGetBytePtr(data), + CFDataGetLength(data)); if (!username) find_username_in_item(item); - SecKeychainItemFreeContent(NULL, buf); + CFRelease(item); + +out: + CFRelease(attrs); + + /* We consider not found to not be an error */ + if (result == errSecItemNotFound) + result = errSecSuccess; + + return result; } -static void delete_internet_password(void) +static OSStatus delete_internet_password(void) { - SecKeychainItemRef item; + CFDictionaryRef attrs; + OSStatus result; /* * Require at least a protocol and host for removal, which is what git @@ -90,25 +180,42 @@ static void delete_internet_password(void) * Keychain manager. */ if (!protocol || !host) - return; + return -1; - if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, 0, NULL, &item)) - return; + attrs = CREATE_SEC_ATTRIBUTES(NULL); + result = SecItemDelete(attrs); + CFRelease(attrs); + + /* We consider not found to not be an error */ + if (result == errSecItemNotFound) + result = errSecSuccess; - SecKeychainItemDelete(item); + return result; } -static void add_internet_password(void) +static OSStatus add_internet_password(void) { + CFDictionaryRef attrs; + OSStatus result; + /* Only store complete credentials */ if (!protocol || !host || !username || !password) - return; + return -1; - if (SecKeychainAddInternetPassword( - KEYCHAIN_ARGS, - KEYCHAIN_ITEM(password), - NULL)) - return; + attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password, + NULL); + + result = SecItemAdd(attrs, NULL); + if (result == errSecDuplicateItem) { + CFDictionaryRef query; + query = CREATE_SEC_ATTRIBUTES(NULL); + result = SecItemUpdate(query, attrs); + CFRelease(query); + } + + CFRelease(attrs); + + return result; } static void read_credential(void) @@ -131,36 +238,52 @@ static void read_credential(void) if (!strcmp(buf, "protocol")) { if (!strcmp(v, "imap")) - protocol = kSecProtocolTypeIMAP; + protocol = kSecAttrProtocolIMAP; else if (!strcmp(v, "imaps")) - protocol = kSecProtocolTypeIMAPS; + protocol = kSecAttrProtocolIMAPS; else if (!strcmp(v, "ftp")) - protocol = kSecProtocolTypeFTP; + protocol = kSecAttrProtocolFTP; else if (!strcmp(v, "ftps")) - protocol = kSecProtocolTypeFTPS; + protocol = kSecAttrProtocolFTPS; else if (!strcmp(v, "https")) - protocol = kSecProtocolTypeHTTPS; + protocol = kSecAttrProtocolHTTPS; else if (!strcmp(v, "http")) - protocol = kSecProtocolTypeHTTP; + protocol = kSecAttrProtocolHTTP; else if (!strcmp(v, "smtp")) - protocol = kSecProtocolTypeSMTP; - else /* we don't yet handle other protocols */ + protocol = kSecAttrProtocolSMTP; + else { + /* we don't yet handle other protocols */ + clear_credential(); exit(0); + } } else if (!strcmp(buf, "host")) { char *colon = strchr(v, ':'); if (colon) { + UInt16 port_i; *colon++ = '\0'; - port = atoi(colon); + port_i = atoi(colon); + port = CFNumberCreate(kCFAllocatorDefault, + kCFNumberShortType, + &port_i); } - host = xstrdup(v); + host = CFStringCreateWithCString(kCFAllocatorDefault, + v, + ENCODING); } else if (!strcmp(buf, "path")) - path = xstrdup(v); + path = CFStringCreateWithCString(kCFAllocatorDefault, + v, + ENCODING); else if (!strcmp(buf, "username")) - username = xstrdup(v); + username = CFStringCreateWithCString( + kCFAllocatorDefault, + v, + ENCODING); else if (!strcmp(buf, "password")) - password = xstrdup(v); + password = CFDataCreate(kCFAllocatorDefault, + (UInt8 *)v, + strlen(v)); /* * Ignore other lines; we don't know what they mean, but * this future-proofs us when later versions of git do @@ -173,6 +296,7 @@ static void read_credential(void) int main(int argc, const char **argv) { + OSStatus result = 0; const char *usage = "usage: git credential-osxkeychain <get|store|erase>"; @@ -182,12 +306,17 @@ int main(int argc, const char **argv) read_credential(); if (!strcmp(argv[1], "get")) - find_internet_password(); + result = find_internet_password(); else if (!strcmp(argv[1], "store")) - add_internet_password(); + result = add_internet_password(); else if (!strcmp(argv[1], "erase")) - delete_internet_password(); + result = delete_internet_password(); /* otherwise, ignore unknown action */ + if (result) + die("failed to %s: %d", argv[1], (int)result); + + clear_credential(); + return 0; }