Skip to content

Commit 167fa55

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 0d46470 commit 167fa55

File tree

2 files changed

+105
-59
lines changed

2 files changed

+105
-59
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: 49 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -102,67 +102,81 @@ private unsafe static int Read(
102102
UIntPtr buf_size,
103103
out UIntPtr bytes_read)
104104
{
105-
GitErrorCode errorCode = GitErrorCode.Error;
106105
bytes_read = UIntPtr.Zero;
107106

108107
SmartSubtransportStream transportStream =
109108
GCHandle.FromIntPtr(Marshal.ReadIntPtr(stream, GitSmartSubtransportStream.GCHandleOffset)).Target as SmartSubtransportStream;
110109

111-
if (transportStream != null &&
112-
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
113123
{
114124
using (UnmanagedMemoryStream memoryStream = new UnmanagedMemoryStream((byte*)buffer, 0,
115125
(long)buf_size.ToUInt64(),
116126
FileAccess.ReadWrite))
117127
{
118-
try
119-
{
120-
long longBytesRead;
121-
122-
int toReturn = transportStream.Read(memoryStream, (long)buf_size.ToUInt64(), out longBytesRead);
123-
124-
bytes_read = new UIntPtr((ulong)Math.Max(0, longBytesRead));
125-
126-
return toReturn;
127-
}
128-
catch (NativeException ex)
129-
{
130-
errorCode = ex.ErrorCode;
131-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
132-
}
133-
catch (Exception ex)
134-
{
135-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
136-
}
128+
long longBytesRead;
129+
130+
int toReturn = transportStream.Read(memoryStream, (long)buf_size.ToUInt64(), out longBytesRead);
131+
132+
bytes_read = new UIntPtr((ulong)Math.Max(0, longBytesRead));
133+
134+
return toReturn;
137135
}
138136
}
139-
140-
return (int)errorCode;
137+
catch (NativeException ex)
138+
{
139+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
140+
return (int)ex.ErrorCode;
141+
}
142+
catch (Exception ex)
143+
{
144+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
145+
return (int)GitErrorCode.Error;
146+
}
141147
}
142148

143149
private static unsafe int Write(IntPtr stream, IntPtr buffer, UIntPtr len)
144150
{
145151
SmartSubtransportStream transportStream =
146152
GCHandle.FromIntPtr(Marshal.ReadIntPtr(stream, GitSmartSubtransportStream.GCHandleOffset)).Target as SmartSubtransportStream;
147153

148-
if (transportStream != null && len.ToUInt64() < (ulong)long.MaxValue)
154+
if (transportStream == null)
155+
{
156+
Proxy.giterr_set_str(GitErrorCategory.Net, "no transport stream provided");
157+
return (int)GitErrorCode.Error;
158+
}
159+
160+
if (len.ToUInt64() >= (ulong)long.MaxValue)
161+
{
162+
Proxy.giterr_set_str(GitErrorCategory.Net, "write length is too large");
163+
return (int)GitErrorCode.Error;
164+
}
165+
166+
try
149167
{
150168
long length = (long)len.ToUInt64();
151169

152170
using (UnmanagedMemoryStream dataStream = new UnmanagedMemoryStream((byte*)buffer, length))
153171
{
154-
try
155-
{
156-
return transportStream.Write(dataStream, length);
157-
}
158-
catch (Exception ex)
159-
{
160-
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
161-
}
172+
return transportStream.Write(dataStream, length);
162173
}
163174
}
164-
165-
return (int)GitErrorCode.Error;
175+
catch (Exception ex)
176+
{
177+
Proxy.giterr_set_str(GitErrorCategory.Net, ex);
178+
return (int)GitErrorCode.Error;
179+
}
166180
}
167181

168182
private static void Free(IntPtr stream)

0 commit comments

Comments
 (0)