Closed Bug 936886 Opened 6 years ago Closed 6 years ago

Account for possibly non-null terminated strings in nsWindowsRegKey::ReadStringValue()

Categories

(Core :: XPCOM, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

David Major pointed out in bug 328655 comment 22 that the string provided by RegQueryValueExW() might not actually be null terminated.
Attached patch patch (obsolete) — Splinter Review
This patch tests the last value written to the string to see if it is a null terminator and either null terminates it or shortens the string by 1 as the extra slot for the null terminator is not needed.


I also noticed that the test for ReadStringValue() (xpcom/tests/TestWinReg.js) appears to be dead code.  I have moved it to a more appropriate location and modified it so it will actually run.

Correction: The previous comment should refer to bug 328755 comment 22.
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Attachment #829841 - Flags: review?(benjamin)
This should also resolve a crash in bug 328755.
Blocks: 328755
It turns out that tryserver does not like test_winreg.js, although further debugging is needed to find the exact cause.

I'm having second thoughts about whether it can be enabled.  In its current state it doesn't perform any tests, instead simply reporting an error if an exception is thrown and calling dump() on strings representing the registry tree structure.  

Beyond writing to the registry and reading the value back, I'm not sure how to properly test this feature as the existence or nature of any registry entry cannot be assumed.
Comment on attachment 829841 [details] [diff] [review]
patch

I'm going to delegate this since I'm not clear on the RegQueryValueEx details. Is the result size the size of the *buffer* or the size of the string?
Attachment #829841 - Flags: review?(benjamin) → review?(dmajor)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> I'm going to delegate this since I'm not clear on the RegQueryValueEx
> details. Is the result size the size of the *buffer* or the size of the
> string?

Both. :-\ On the way in, that parameter tells the API how much room is in the buffer. On the way out, it tells the caller how much was actually written. (Or in some cases, how much space you *would* need)
Comment on attachment 829841 [details] [diff] [review]
patch

Review of attachment 829841 [details] [diff] [review]:
-----------------------------------------------------------------

If RegQueryValueExW returned a REG_MULTI_SZ that was missing one or both of the required nulls, then this code wouldn't produce a true multi-string -- even with the added check, there would only be one null at the end. Maybe we don't care... it seems many callers are totally unprepared for such a string format, so they're probably already broken today. I'm kind of shocked that this method allows REG_MULTI_SZ at all.
Comment on attachment 829841 [details] [diff] [review]
patch

Review of attachment 829841 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsWindowsRegKey.cpp
@@ +300,5 @@
>                          &size);
>  
> +  if (result.CharAt(resultLen-1)) {
> +    // not null terminated
> +    *result.EndWriting() = 0;

I'm not very familiar with the string APIs -- are you allowed to write to this position? From poking around the codebase, it seems many callers treat EndWriting as being the first character beyond the end of the string, rather than the last character within the string.
Attached patch reg.diff (obsolete) — Splinter Review
EndWriting() should always point to the null terminator, which is not counted in determining the length of the string.

I have since discovered that the null terminator is appropriately positioned when SetLength() is called, so the call to EndWriting() was redundant and has been removed.
Attachment #829841 - Attachment is obsolete: true
Attachment #829841 - Flags: review?(dmajor)
Attachment #832145 - Flags: review?(dmajor)
Attached patch reg.diff (obsolete) — Splinter Review
comment fix
Attachment #832145 - Attachment is obsolete: true
Attachment #832145 - Flags: review?(dmajor)
Attachment #832146 - Flags: review?(dmajor)
Comment on attachment 832146 [details] [diff] [review]
reg.diff

Review of attachment 832146 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay; I got caught up in mochitest investigations last week. I've applied this patch and stepped through and it looks good to me.

::: xpcom/ds/nsWindowsRegKey.cpp
@@ +303,5 @@
> +    // The string passed to us had a null terminator in the final position.
> +    result.Truncate(resultLen-1);
> +  }
> +
> +

Nit: I would collapse these double blanks down to just one.
Attachment #832146 - Flags: review?(dmajor) → review+
Attached patch reg.diffSplinter Review
Whitespace fix applied.
Attachment #832146 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7842ca0705f5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.