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.

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 for JsPointerToString and JsCreateString #6669 #6737

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions bin/NativeTests/JsRTApiTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@ namespace JsRTApiTest
JsValueRef value2 = JS_INVALID_REFERENCE;
REQUIRE(JsPointerToString(_u("value1"), wcslen(_u("value1")), &value2) == JsNoError);

// Test JsPointerToString and JsCreateString on NULL inputs
JsValueRef nullStr;
REQUIRE(JsPointerToString(NULL, 0, &nullStr) == JsNoError);

JsValueRef nullFname;
REQUIRE(JsCreateString(NULL, 0, &nullFname) == JsNoError);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please could you use spaces instead of tabs for the indent (here and other files) - our code base uses spaces consistently.

REQUIRE(JsSetProperty(object, name1, value1, true) == JsNoError);
REQUIRE(JsSetProperty(object, name2, value2, true) == JsNoError);

Expand Down
9 changes: 7 additions & 2 deletions lib/Jsrt/ChakraCommonWindows.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,12 @@
/// Creates a string value from a string pointer.
/// </summary>
/// <remarks>
/// Requires an active script context.
/// <para>
/// Requires an active script context.
/// </para>
/// <para>
/// Providing length of 0 will ignore input string pointer and return a Js empty string.
/// </para>
/// </remarks>
/// <param name="stringValue">The string pointer to convert to a string value.</param>
/// <param name="stringLength">The length of the string to convert.</param>
Expand All @@ -333,7 +338,7 @@
/// </returns>
CHAKRA_API
JsPointerToString(
_In_reads_(stringLength) const wchar_t *stringValue,
_In_reads_opt_(stringLength) const wchar_t *stringValue,
_In_ size_t stringLength,
_Out_ JsValueRef *value);

Expand Down
5 changes: 4 additions & 1 deletion lib/Jsrt/ChakraCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,9 @@ typedef bool (CHAKRA_CALLBACK * JsSerializedLoadScriptCallback)
/// <para>
/// Input string can be either ASCII or Utf8
/// </para>
/// <para>
/// Providing length of 0 will ignore input string pointer and return a Js empty string.
/// </para>
/// </remarks>
/// <param name="content">Pointer to string memory.</param>
/// <param name="length">Number of bytes within the string</param>
Expand All @@ -514,7 +517,7 @@ typedef bool (CHAKRA_CALLBACK * JsSerializedLoadScriptCallback)
/// </returns>
CHAKRA_API
JsCreateString(
_In_ const char *content,
_In_opt_ const char *content,
_In_ size_t length,
_Out_ JsValueRef *value);

Expand Down
7 changes: 6 additions & 1 deletion lib/Jsrt/Core/JsrtCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -868,10 +868,15 @@ static void CastCopy(const SrcChar* src, DstChar* dst, size_t count)
}

CHAKRA_API JsCreateString(
_In_ const char *content,
_In_opt_ const char *content,
_In_ size_t length,
_Out_ JsValueRef *value)
{
if (length == 0)
{
content = "";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please could you bring this in line with the way you've done JsPointerToString below? If length = 0, skip the PARAM_NOT_NULL(content); and then use scriptContext->GetLibrary()->GetEmptyString(); instead of the logic on lines 896 and 898 below.

Copy link
Author

@dagarxji dagarxji Aug 13, 2021

Choose a reason for hiding this comment

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

Oh, I see. I had issues with GetEmptyString() because I was including line 898. Executing 896 and 898 only when length isn't zero fixed this. Thank you.


PARAM_NOT_NULL(content);
PARAM_NOT_NULL(value);
*value = JS_INVALID_REFERENCE;
Expand Down
30 changes: 19 additions & 11 deletions lib/Jsrt/Jsrt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1229,20 +1229,28 @@ CHAKRA_API JsGetStringLength(_In_ JsValueRef value, _Out_ int *length)
END_JSRT_NO_EXCEPTION
}

CHAKRA_API JsPointerToString(_In_reads_(stringLength) const WCHAR *stringValue, _In_ size_t stringLength, _Out_ JsValueRef *string)
CHAKRA_API JsPointerToString(_In_reads_opt_(stringLength) const WCHAR *stringValue, _In_ size_t stringLength, _Out_ JsValueRef *string)
{
return ContextAPINoScriptWrapper([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTCreateString, stringValue, stringLength);

PARAM_NOT_NULL(stringValue);
PARAM_NOT_NULL(string);

if (!Js::IsValidCharCount(stringLength))
{
Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);
}

*string = Js::JavascriptString::NewCopyBuffer(stringValue, static_cast<charcount_t>(stringLength), scriptContext);

PARAM_NOT_NULL(string);

if (stringLength == 0)
{
*string = scriptContext->GetLibrary()->GetEmptyString();
}
else
{
PARAM_NOT_NULL(stringValue);


if (!Js::IsValidCharCount(stringLength))
{
Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);
}
*string = Js::JavascriptString::NewCopyBuffer(stringValue, static_cast<charcount_t>(stringLength), scriptContext);
}

PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, string);

Expand Down