Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix insecureAPI, NonLocalizedStringChecker, NewDeleteLeaks macOS clang errors #31333

Closed
wants to merge 1 commit into from
+13 −7
Closed

Fix insecureAPI, NonLocalizedStringChecker, NewDeleteLeaks macOS clang errors #31333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions fml/synchronization/waitable_event_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "flutter/fml/synchronization/waitable_event.h"

#include <stdlib.h>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why isn't this include sufficient?

../../flutter/fml/synchronization/waitable_event_unittest.cc:35:57: error: use of undeclared identifier 'arc4random'
      TimeDelta::FromMilliseconds(static_cast<unsigned>(arc4random()) % 20u);

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8822761656441545457/+/u/build_host_debug_unopt/stdout

#include <atomic>
#include <cstddef>
#include <cstdint>
Expand Down Expand Up @@ -31,7 +32,7 @@ void SleepFor(TimeDelta duration) {

void EpsilonRandomSleep() {
TimeDelta duration =
TimeDelta::FromMilliseconds(static_cast<unsigned>(rand()) % 20u);
TimeDelta::FromMilliseconds(static_cast<unsigned>(arc4random()) % 20u);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter for these unit tests, but can't hurt.

error: Function 'rand' is obsolete because it implements a poor random number generator.  Use 'arc4random' instead [clang-analyzer-security.insecureAPI.rand,-warnings-as-errors]

SleepFor(duration);
}

Expand Down Expand Up @@ -73,7 +74,7 @@ TEST(AutoResetWaitableEventTest, MultipleWaiters) {
std::vector<std::thread> threads;
for (size_t j = 0u; j < 4u; j++) {
threads.push_back(std::thread([&ev, &wake_count]() {
if (rand() % 2 == 0) {
if (arc4random() % 2 == 0) {
ev.Wait();
} else {
EXPECT_FALSE(ev.WaitWithTimeout(kActionTimeout));
Expand Down Expand Up @@ -158,7 +159,7 @@ TEST(ManualResetWaitableEventTest, SignalMultiple) {
threads.push_back(std::thread([&ev]() {
EpsilonRandomSleep();

if (rand() % 2 == 0) {
if (arc4random() % 2 == 0) {
ev.Wait();
} else {
EXPECT_FALSE(ev.WaitWithTimeout(kActionTimeout));
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacDelegateTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,4 +199,4 @@ void DispatchMacOSNotification(gfx::NativeViewAccessible native_node,
[engine shutDownEngine];
}

} // flutter::testing
} // namespace flutter::testing
2 changes: 1 addition & 1 deletion shell/platform/darwin/macos/framework/Source/FlutterEmbedderKeyResponder.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,12 @@ static uint64_t GetLogicalKeyForEvent(NSEvent* event, uint64_t physicalKey) {
uint32_t* keyLabel = DecodeUtf16(keyLabelUtf16, &keyLabelLength);
if (keyLabelLength == 1) {
uint32_t keyLabelChar = *keyLabel;
delete[] keyLabel;
NSCAssert(!IsControlCharacter(keyLabelChar) && !IsUnprintableKey(keyLabelChar),
@"Unexpected control or unprintable keylabel 0x%x", keyLabelChar);
NSCAssert(keyLabelChar <= 0x10FFFF, @"Out of range keylabel 0x%x", keyLabelChar);
character = keyLabelChar;
}
delete[] keyLabel;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

error: Potential leak of memory pointed to by 'keyLabel' [clang-analyzer-cplusplus.NewDeleteLeaks,-warnings-as-errors]

}
if (character != 0) {
return KeyOfPlane(toLower(character), kUnicodePlane);
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/macos/framework/Source/FlutterEmbedderKeyResponderUnittests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ - (instancetype)initWithEvent:(const FlutterKeyEvent*)event
if (event->character != nullptr) {
size_t len = strlen(event->character);
char* character = new char[len + 1];
strcpy(character, event->character);
strlcpy(character, event->character, sizeof(character));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter for this unit test, but can't hurt.

error: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy,-warnings-as-errors]

_data->character = character;
}
_callback = callback;
Expand Down
7 changes: 6 additions & 1 deletion shell/platform/darwin/macos/framework/Source/FlutterPlatformNodeDelegateMacTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,12 @@
FlutterViewController* viewController = [[FlutterViewController alloc] initWithProject:project];
[viewController loadView];
[engine setViewController:viewController];

// Unit test localization is unnecessary.
// NOLINTBEGIN(clang-analyzer-optin.osx.cocoa.localizability.NonLocalizedStringChecker)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

error: User-facing text should use localized string macro [clang-analyzer-optin.osx.cocoa.localizability.NonLocalizedStringChecker,-warnings-as-errors]

viewController.textInputPlugin.string = @"textfield";
// NOLINTEND(clang-analyzer-optin.osx.cocoa.localizability.NonLocalizedStringChecker)

// Creates a NSWindow so that the native text field can become first responder.
NSWindow* window = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600)
styleMask:NSBorderlessWindowMask
Expand Down Expand Up @@ -291,4 +296,4 @@
EXPECT_EQ([native_text_field.stringValue isEqualToString:@"textfield"], YES);
}

} // flutter::testing
} // namespace flutter::testing
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a warning, but fix it anyway:

warning: namespace 'flutter::testing' ends with an unrecognized comment [google-readability-namespace-comments]