Skip to content

Commit 5a64349

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 9d7dd69 commit 5a64349

File tree

2 files changed

+98
-59
lines changed

2 files changed

+98
-59
lines changed

‎LibGit2Sharp/SmartSubtransport.cs‎

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,11 @@ 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

@@ -134,6 +139,12 @@ public int AcquireCredentials(out Credentials cred, string user, params Type[] m
134139
}
135140
}
136141

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+
137148
/// <summary>
138149
/// Invoked by libgit2 to create a connection using this subtransport.
139150
/// </summary>
@@ -209,43 +220,57 @@ private static int Action(
209220
SmartSubtransport t = GCHandle.FromIntPtr(Marshal.ReadIntPtr(subtransport, GitSmartSubtransport.GCHandleOffset)).Target as SmartSubtransport;
210221
String urlAsString = LaxUtf8Marshaler.FromNative(url);
211222

212-
if (null != t &&
213-
!String.IsNullOrEmpty(urlAsString))
223+
if (t == null)
214224
{
215-
try
216-
{
217-
stream = t.Action(urlAsString, action).GitSmartTransportStreamPointer;
225+
Proxy.git_error_set_str(GitErrorCategory.Net, "no subtransport provided");
226+
return (int)GitErrorCode.Error;
227+
}
218228

219-
return 0;
220-
}
221-
catch (Exception ex)
222-
{
223-
Proxy.git_error_set_str(GitErrorCategory.Net, ex);
224-
}
229+
if (String.IsNullOrEmpty(urlAsString))
230+
{
231+
urlAsString = t.LastActionUrl;
232+
}
233+
234+
if (String.IsNullOrEmpty(urlAsString))
235+
{
236+
Proxy.git_error_set_str(GitErrorCategory.Net, "no url provided");
237+
return (int)GitErrorCode.Error;
225238
}
226239

227-
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.git_error_set_str(GitErrorCategory.Net, ex);
249+
return (int)GitErrorCode.Error;
250+
}
228251
}
229252

230253
private static int Close(IntPtr subtransport)
231254
{
232255
SmartSubtransport t = GCHandle.FromIntPtr(Marshal.ReadIntPtr(subtransport, GitSmartSubtransport.GCHandleOffset)).Target as SmartSubtransport;
233256

234-
if (null != t)
257+
if (t == null)
235258
{
236-
try
237-
{
238-
t.Close();
239-
240-
return 0;
241-
}
242-
catch (Exception ex)
243-
{
244-
Proxy.git_error_set_str(GitErrorCategory.Net, ex);
245-
}
259+
Proxy.git_error_set_str(GitErrorCategory.Net, "no subtransport provided");
260+
return (int)GitErrorCode.Error;
246261
}
247262

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

251276
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.git_error_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.git_error_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.git_error_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.git_error_set_str(GitErrorCategory.Net, ex);
140+
return (int)ex.ErrorCode;
141+
}
142+
catch (Exception ex)
143+
{
144+
Proxy.git_error_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.git_error_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.git_error_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.git_error_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.git_error_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)