Skip to content

Commit 10aa8c2

Browse files
ethomsonethomson
committed
smart subtransports: improve safety
Throwing an exception in a native callback is pointless; instead, carefully protect the entry points to return a native error code (-1) on failure. Wrap everything else in a try/catch to propagate error codes.
1 parent e736ba1 commit 10aa8c2

File tree

2 files changed

+97
-50
lines changed

2 files changed

+97
-50
lines changed

‎LibGit2Sharp/SmartSubtransport.cs‎

Lines changed: 56 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,21 @@ public int CertificateCheck(Certificate cert, bool valid, string hostname)
8181
Marshal.FreeHGlobal(certPtr);
8282
}
8383

84+
if (ret > 0)
85+
{
86+
ret = valid ? 0 : -1;
87+
}
88+
8489
return ret;
8590
}
8691

92+
/// <summary>
93+
/// Call the credential acquisition callback
94+
/// </summary>
95+
/// <param name="cred">Credential data to populate</param>
96+
/// <param name="user">Username from the URL, if any</param>
97+
/// <param name="methods">Supported methods</param>
98+
/// <returns></returns>
8799
public int AcquireCredentials(out Credentials cred, string user, params Type[] methods)
88100
{
89101
// Convert the user-provided types to libgit2's flags
@@ -127,6 +139,12 @@ public int AcquireCredentials(out Credentials cred, string user, params Type[] m
127139
}
128140
}
129141

142+
/// <summary>
143+
/// libgit2 will call an action back with a null url to indicate that
144+
/// it should re-use the prior url; store the url so that we can replay.
145+
/// </summary>
146+
private string LastActionUrl { get; set; }
147+
130148
/// <summary>
131149
/// Invoked by libgit2 to create a connection using this subtransport.
132150
/// </summary>
@@ -202,43 +220,57 @@ private static int Action(
202220
SmartSubtransport t = GCHandle.FromIntPtr(Marshal.ReadIntPtr(subtransport, GitSmartSubtransport.GCHandleOffset)).Target as SmartSubtransport;
203221
String urlAsString = LaxUtf8Marshaler.FromNative(url);
204222

205-
if (null != t &&
206-
!String.IsNullOrEmpty(urlAsString))
223+
if (t == null)
207224
{
208-
try
209-
{
210-
stream = t.Action(urlAsString, action).GitSmartTransportStreamPointer;
225+
Proxy.giterr_set_str(GitErrorCategory.Net, "no subtransport provided");
226+
return (int)GitErrorCode.Error;
227+
}
211228

212-
return 0;
213-
}
214-
catch (Exception ex)
215-
{
216-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
217-
}
229+
if (String.IsNullOrEmpty(urlAsString))
230+
{
231+
urlAsString = t.LastActionUrl;
232+
}
233+
234+
if (String.IsNullOrEmpty(urlAsString))
235+
{
236+
Proxy.giterr_set_str(GitErrorCategory.Net, "no url provided");
237+
return (int)GitErrorCode.Error;
218238
}
219239

220-
return (int)GitErrorCode.Error;
240+
try
241+
{
242+
stream = t.Action(urlAsString, action).GitSmartTransportStreamPointer;
243+
t.LastActionUrl = urlAsString;
244+
return 0;
245+
}
246+
catch (Exception ex)
247+
{
248+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
249+
return (int)GitErrorCode.Error;
250+
}
221251
}
222252

223253
private static int Close(IntPtr subtransport)
224254
{
225255
SmartSubtransport t = GCHandle.FromIntPtr(Marshal.ReadIntPtr(subtransport, GitSmartSubtransport.GCHandleOffset)).Target as SmartSubtransport;
226256

227-
if (null != t)
257+
if (t == null)
228258
{
229-
try
230-
{
231-
t.Close();
232-
233-
return 0;
234-
}
235-
catch (Exception ex)
236-
{
237-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
238-
}
259+
Proxy.giterr_set_str(GitErrorCategory.Net, "no subtransport provided");
260+
return (int)GitErrorCode.Error;
239261
}
240262

241-
return (int)GitErrorCode.Error;
263+
try
264+
{
265+
t.Close();
266+
267+
return 0;
268+
}
269+
catch (Exception ex)
270+
{
271+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
272+
return (int)GitErrorCode.Error;
273+
}
242274
}
243275

244276
private static void Free(IntPtr subtransport)

‎LibGit2Sharp/SmartSubtransportStream.cs‎

Lines changed: 41 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -107,56 +107,71 @@ private unsafe static int Read(
107107
SmartSubtransportStream transportStream =
108108
GCHandle.FromIntPtr(Marshal.ReadIntPtr(stream, GitSmartSubtransportStream.GCHandleOffset)).Target as SmartSubtransportStream;
109109

110-
if (transportStream != null &&
111-
buf_size.ToUInt64() < (ulong)long.MaxValue)
110+
if (transportStream == null)
111+
{
112+
Proxy.giterr_set_str(GitErrorCategory.Net, "no transport stream provided");
113+
return (int)GitErrorCode.Error;
114+
}
115+
116+
if (buf_size.ToUInt64() >= (ulong)long.MaxValue)
117+
{
118+
Proxy.giterr_set_str(GitErrorCategory.Net, "buffer size is too large");
119+
return (int)GitErrorCode.Error;
120+
}
121+
122+
try
112123
{
113124
using (UnmanagedMemoryStream memoryStream = new UnmanagedMemoryStream((byte*)buffer, 0,
114125
(long)buf_size.ToUInt64(),
115126
FileAccess.ReadWrite))
116127
{
117-
try
118-
{
119-
long longBytesRead;
128+
long longBytesRead;
120129

121-
int toReturn = transportStream.Read(memoryStream, (long)buf_size.ToUInt64(), out longBytesRead);
130+
int toReturn = transportStream.Read(memoryStream, (long)buf_size.ToUInt64(), out longBytesRead);
122131

123-
bytes_read = new UIntPtr((ulong)Math.Max(0, longBytesRead));
132+
bytes_read = new UIntPtr((ulong)Math.Max(0, longBytesRead));
124133

125-
return toReturn;
126-
}
127-
catch (Exception ex)
128-
{
129-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
130-
}
134+
return toReturn;
131135
}
132136
}
133-
134-
return (int)GitErrorCode.Error;
137+
catch (Exception ex)
138+
{
139+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
140+
return (int)GitErrorCode.Error;
141+
}
135142
}
136143

137144
private static unsafe int Write(IntPtr stream, IntPtr buffer, UIntPtr len)
138145
{
139146
SmartSubtransportStream transportStream =
140147
GCHandle.FromIntPtr(Marshal.ReadIntPtr(stream, GitSmartSubtransportStream.GCHandleOffset)).Target as SmartSubtransportStream;
141148

142-
if (transportStream != null && len.ToUInt64() < (ulong)long.MaxValue)
149+
if (transportStream == null)
150+
{
151+
Proxy.giterr_set_str(GitErrorCategory.Net, "no transport stream provided");
152+
return (int)GitErrorCode.Error;
153+
}
154+
155+
if (len.ToUInt64() >= (ulong)long.MaxValue)
156+
{
157+
Proxy.giterr_set_str(GitErrorCategory.Net, "write length is too large");
158+
return (int)GitErrorCode.Error;
159+
}
160+
161+
try
143162
{
144163
long length = (long)len.ToUInt64();
145164

146165
using (UnmanagedMemoryStream dataStream = new UnmanagedMemoryStream((byte*)buffer, length))
147166
{
148-
try
149-
{
150-
return transportStream.Write(dataStream, length);
151-
}
152-
catch (Exception ex)
153-
{
154-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
155-
}
167+
return transportStream.Write(dataStream, length);
156168
}
157169
}
158-
159-
return (int)GitErrorCode.Error;
170+
catch (Exception ex)
171+
{
172+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
173+
return (int)GitErrorCode.Error;
174+
}
160175
}
161176

162177
private static void Free(IntPtr stream)

0 commit comments

Comments
 (0)