From faa8946056a9411ef3b98662cfe6787f6590f0fe Mon Sep 17 00:00:00 2001 From: Pete Markowsky Date: Mon, 14 Feb 2022 13:51:29 -0500 Subject: [PATCH] Fix: remediate a crash in santametricservice (#729) * Fix issue with task cancelation. * Make export timeouts configurable. This allows an export timeout to be set via configuration and eases testing. --- Source/common/SNTConfigurator.h | 5 ++ Source/common/SNTConfigurator.m | 12 ++++ Source/santametricservice/Writers/BUILD | 1 + .../Writers/SNTMetricHTTPWriter.m | 69 +++++++++++-------- .../Writers/SNTMetricHTTPWriterTest.m | 21 +++++- docs/deployment/configuration.md | 3 +- 6 files changed, 79 insertions(+), 32 deletions(-) diff --git a/Source/common/SNTConfigurator.h b/Source/common/SNTConfigurator.h index e292634db..65e0d893a 100644 --- a/Source/common/SNTConfigurator.h +++ b/Source/common/SNTConfigurator.h @@ -438,6 +438,11 @@ /// @property(readonly, nonatomic) NSUInteger metricExportInterval; +/// +/// Duration in seconds for metrics export timeout. Defaults to 30; +/// +@property(readonly, nonatomic) NSUInteger metricExportTimeout; + /// /// Retrieve an initialized singleton configurator object using the default file path. /// diff --git a/Source/common/SNTConfigurator.m b/Source/common/SNTConfigurator.m index ce5741f7e..1dc491c46 100644 --- a/Source/common/SNTConfigurator.m +++ b/Source/common/SNTConfigurator.m @@ -110,6 +110,7 @@ @implementation SNTConfigurator static NSString *const kMetricFormat = @"MetricFormat"; static NSString *const kMetricURL = @"MetricURL"; static NSString *const kMetricExportInterval = @"MetricExportInterval"; +static NSString *const kMetricExportTimeout = @"MetricExportTimeout"; static NSString *const kMetricExtraLabels = @"MetricExtraLabels"; // The keys managed by a sync server. @@ -193,6 +194,7 @@ - (instancetype)init { kMetricFormat : string, kMetricURL : string, kMetricExportInterval : number, + kMetricExportTimeout : number, kMetricExtraLabels : dictionary, }; _defaults = [NSUserDefaults standardUserDefaults]; @@ -759,6 +761,16 @@ - (NSUInteger)metricExportInterval { return [configuredInterval unsignedIntegerValue]; } +// Returns a default value of 30 (for 30 seconds). +- (NSUInteger)metricExportTimeout { + NSNumber *configuredInterval = self.configState[kMetricExportTimeout]; + + if (configuredInterval == nil) { + return 30; + } + return [configuredInterval unsignedIntegerValue]; +} + - (NSDictionary *)extraMetricLabels { return self.configState[kMetricExtraLabels]; } diff --git a/Source/santametricservice/Writers/BUILD b/Source/santametricservice/Writers/BUILD index 1cc3e3435..b8699a548 100644 --- a/Source/santametricservice/Writers/BUILD +++ b/Source/santametricservice/Writers/BUILD @@ -39,6 +39,7 @@ objc_library( ], deps = [ ":SNTMetricWriter", + "//Source/common:SNTConfigurator", "//Source/common:SNTLogging", "@MOLAuthenticatingURLSession", ], diff --git a/Source/santametricservice/Writers/SNTMetricHTTPWriter.m b/Source/santametricservice/Writers/SNTMetricHTTPWriter.m index e80b37ba9..25e7a7139 100644 --- a/Source/santametricservice/Writers/SNTMetricHTTPWriter.m +++ b/Source/santametricservice/Writers/SNTMetricHTTPWriter.m @@ -14,6 +14,7 @@ #import #include +#import "Source/common/SNTConfigurator.h" #import "Source/common/SNTLogging.h" #import "Source/santametricservice/Writers/SNTMetricHTTPWriter.h" @@ -49,37 +50,45 @@ - (BOOL)write:(NSArray *)metrics toURL:(NSURL *)url error:(NSError **) dispatch_group_enter(requests); request.HTTPBody = (NSData *)value; - [[_session dataTaskWithRequest:request - completionHandler:^(NSData *_Nullable data, NSURLResponse *_Nullable response, - NSError *_Nullable err) { - if (err != nil) { - _blockError = err; - *stop = YES; - } else if (response == nil) { - *stop = YES; - } else if ([response isKindOfClass:[NSHTTPURLResponse class]]) { - NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response; - - // Check HTTP error codes and create errors for any non-200. - if (httpResponse && httpResponse.statusCode != 200) { - _blockError = [[NSError alloc] - initWithDomain:@"com.google.santa.metricservice.writers.http" - code:httpResponse.statusCode - userInfo:@{ - NSLocalizedDescriptionKey : [NSString - stringWithFormat:@"received http status code %ld from %@", - httpResponse.statusCode, url] - }]; - *stop = YES; - } - } - dispatch_group_leave(requests); - }] resume]; - - // Wait up to 30 seconds for the request to complete. - if (dispatch_group_wait(requests, (int64_t)(30.0 * NSEC_PER_SEC)) != 0) { + NSURLSessionDataTask *task = [_session + dataTaskWithRequest:request + completionHandler:^(NSData *_Nullable data, NSURLResponse *_Nullable response, + NSError *_Nullable err) { + if (err != nil) { + _blockError = err; + *stop = YES; + } else if (response == nil) { + *stop = YES; + } else if ([response isKindOfClass:[NSHTTPURLResponse class]]) { + NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response; + + // Check HTTP error codes and create errors for any non-200. + if (httpResponse && httpResponse.statusCode != 200) { + _blockError = [[NSError alloc] + initWithDomain:@"com.google.santa.metricservice.writers.http" + code:httpResponse.statusCode + userInfo:@{ + NSLocalizedDescriptionKey : + [NSString stringWithFormat:@"received http status code %ld from %@", + httpResponse.statusCode, url] + }]; + *stop = YES; + } + } + dispatch_group_leave(requests); + }]; + + [task resume]; + + SNTConfigurator *config = [SNTConfigurator configurator]; + int64_t timeout = (int64_t)config.metricExportTimeout; + + // Wait up to timeout seconds for the request to complete. + if (dispatch_group_wait(requests, (timeout * NSEC_PER_SEC)) != 0) { + [task cancel]; NSString *errMsg = - [NSString stringWithFormat:@"HTTP request to %@ timed out after 30 seconds", url]; + [NSString stringWithFormat:@"HTTP request to %@ timed out after %lu seconds", url, + (unsigned long)timeout]; _blockError = [[NSError alloc] initWithDomain:@"com.google.santa.metricservice.writers.http" code:ETIMEDOUT diff --git a/Source/santametricservice/Writers/SNTMetricHTTPWriterTest.m b/Source/santametricservice/Writers/SNTMetricHTTPWriterTest.m index 1499a8ec8..0376f036c 100644 --- a/Source/santametricservice/Writers/SNTMetricHTTPWriterTest.m +++ b/Source/santametricservice/Writers/SNTMetricHTTPWriterTest.m @@ -4,6 +4,7 @@ #import #import +#import "Source/common/SNTConfigurator.h" #import "Source/santametricservice/Writers/SNTMetricHTTPWriter.h" @interface SNTMetricHTTPWriterTest : XCTestCase @@ -12,6 +13,7 @@ @interface SNTMetricHTTPWriterTest : XCTestCase @property id mockMOLAuthenticatingURLSession; @property NSMutableArray *mockResponses; @property SNTMetricHTTPWriter *httpWriter; +@property id mockConfigurator; @end @implementation SNTMetricHTTPWriterTest @@ -28,6 +30,9 @@ - (void)setUp { self.httpWriter = [[SNTMetricHTTPWriter alloc] init]; self.mockResponses = [[NSMutableArray alloc] init]; + self.mockConfigurator = OCMClassMock([SNTConfigurator class]); + OCMStub([self.mockConfigurator configurator]).andReturn(self.mockConfigurator); + // This must be marked __unsafe_unretained because we're going to store into // it using NSInvocation's getArgument:atIndex: method which takes a void* // to populate. If we don't mark the variable __unsafe_unretained it will @@ -181,4 +186,18 @@ - (void)testEnsurePassingNilOrNullErrorDoesNotCrash { XCTAssertFalse(result); } -@end + +- (void)testEnsureTimeoutsDoNotCrashWriter { + NSURL *url = [NSURL URLWithString:@"http://localhost:11444"]; + + // Queue up two responses for nil and NULL. + [self createMockResponseWithURL:url withCode:400 withData:nil withError:nil]; + // Set the timeout to 0 second + OCMStub([self.mockConfigurator metricExportTimeout]).andReturn(0); + + NSData *JSONdata = [@"{\"foo\": \"bar\"}\r\n" dataUsingEncoding:NSUTF8StringEncoding]; + + BOOL result = [self.httpWriter write:@[ JSONdata ] toURL:url error:nil]; + XCTAssertEqual(NO, result); +} +@end \ No newline at end of file diff --git a/docs/deployment/configuration.md b/docs/deployment/configuration.md index 03308cf95..2f38cf8d8 100644 --- a/docs/deployment/configuration.md +++ b/docs/deployment/configuration.md @@ -56,7 +56,8 @@ also known as mobileconfig files, which are in an Apple-specific XML format. | EnableMachineIDDecoration | Bool | If YES, this appends the MachineID to the end of each log line. Defaults to NO. | | MetricFormat | String | Format to export metrics as, supported formats are "rawjson" for a single JSON blob and "monarchjson" for a format consumable by Google's Monarch tooling. Defaults to "". | | MetricURL | String | URL describing where monitoring metrics should be exported. | -| MetricExportInterval | Integer | Number of seconds to wait between exporting metrics. Defaults to 30. +| MetricExportInterval | Integer | Number of seconds to wait between exporting metrics. Defaults to 30. | +| MetricExportTimeout | Integer | Number of seconds to wait before a timeout occurs when exporting metrics. Defaults to 30. | | MetricExtraLabels | Dictionary | A map of key value pairs to add to all metric root labels. (e.g. a=b,c=d) defaults to @{}). If a previously set key (e.g. host_name is set to "" then the key is remove from the metric root labels. Alternatively if a value is set for an existing key then the new value will override the old. |