Closed Bug 1120937 Opened 5 years ago Closed 5 years ago

TEST-UNEXPECTED-TIMEOUT | security/manager/ssl/tests/unit/test_pkcs11_insert_remove.js | Test timed out

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Keywords: valgrind)

Attachments

(2 files)

This happened on linux64 opt builds when turning jemalloc 3 on, but seeing how I fixed it, I wonder how it didn't happen before. Also, it goes away when I turn off free() poisoning.

Essentially, the pkcs11 test module doesn't fill the session number in OpenSession, and that ends up breaking stuff.

Similarly, while they're not involved in this particular breakage, string data fields are not being initialized correctly. When reading them from the end like in PK11_MakeString, which can contain random values, that ends up with garbage copied.
Assignee: nobody → mh+mozilla
Attachment #8548194 - Flags: review?(brian)
BTW, valgrind was complaining about all those that I'm fixing here, and doesn't complain anymore with the patch applied.
Attachment #8548194 - Flags: review?(brian) → review?(dkeeler)
Comment on attachment 8548194 [details] [diff] [review]
Properly initialize string fields and session field from the PKCS#11 test module

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

Thanks for figuring this out!
To be safe, I think we should do a similar thing in Test_C_GetMechanismInfo (should just be able to memset(pInfo, 0, sizeof(CK_MECHANISM_INFO));)
Is the output from the automated valgrind runs available anywhere? It's sad that this information was available yet I was unaware of it for so long...

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp
@@ +44,5 @@
>    pInfo->cryptokiVersion = CryptokiVersion;
>    static_assert(sizeof(TestManufacturerID) <= sizeof(pInfo->manufacturerID),
>                  "TestManufacturerID too long - make it shorter");
>    memcpy(pInfo->manufacturerID, TestManufacturerID, sizeof(TestManufacturerID));
> +  memset(pInfo->manufacturerID + sizeof(TestManufacturerID), 0,

These memsets all look correct to me, but how about just calling memset(pInfo, 0, sizeof(CK_INFO)); (or CK_SLOT_INFO, etc., as appropriate) once at the top (after the null check)?

@@ +188,5 @@
>    return CKR_FUNCTION_NOT_SUPPORTED;
>  }
>  
>  CK_RV Test_C_OpenSession(CK_SLOT_ID, CK_FLAGS, CK_VOID_PTR, CK_NOTIFY,
> +                         CK_SESSION_HANDLE_PTR session)

I'd prefer this be called 'phSession' to match security/nss/lib/util/pkcs11f.h
Attachment #8548194 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8548194 [details] [diff] [review]
> Properly initialize string fields and session field from the PKCS#11 test
> module
> 
> Review of attachment 8548194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for figuring this out!
> To be safe, I think we should do a similar thing in Test_C_GetMechanismInfo
> (should just be able to memset(pInfo, 0, sizeof(CK_MECHANISM_INFO));)
> Is the output from the automated valgrind runs available anywhere? It's sad
> that this information was available yet I was unaware of it for so long...

The valgrind output didn't come from automation. I ran valgrind on the xpcshell test that was failed when jemalloc 3 was enabled.

> ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp
> @@ +44,5 @@
> >    pInfo->cryptokiVersion = CryptokiVersion;
> >    static_assert(sizeof(TestManufacturerID) <= sizeof(pInfo->manufacturerID),
> >                  "TestManufacturerID too long - make it shorter");
> >    memcpy(pInfo->manufacturerID, TestManufacturerID, sizeof(TestManufacturerID));
> > +  memset(pInfo->manufacturerID + sizeof(TestManufacturerID), 0,
> 
> These memsets all look correct to me, but how about just calling
> memset(pInfo, 0, sizeof(CK_INFO)); (or CK_SLOT_INFO, etc., as appropriate)
> once at the top (after the null check)?

Relatedly, why doesn't the caller do that in NSS (or call calloc to get those buffers)?
That said, aren't some of these functions like the one for token called with an already filled slot info?
Keywords: valgrind
(In reply to Mike Hommey [:glandium] from comment #4)
> Relatedly, why doesn't the caller do that in NSS (or call calloc to get
> those buffers)?

calloc isn't used because they're generally stack-allocated. As for why they're not initialized to zero, it looks like this is old, crufty Netscape code that nobody checked for problems like this.

(In reply to Mike Hommey [:glandium] from comment #5)
> That said, aren't some of these functions like the one for token called with
> an already filled slot info?

I had a look around and couldn't find any examples of this. What callsites are you concerned about?
(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Relatedly, why doesn't the caller do that in NSS (or call calloc to get
> > those buffers)?
> 
> calloc isn't used because they're generally stack-allocated. As for why
> they're not initialized to zero, it looks like this is old, crufty Netscape
> code that nobody checked for problems like this.

How about fixing that, then?

> (In reply to Mike Hommey [:glandium] from comment #5)
> > That said, aren't some of these functions like the one for token called with
> > an already filled slot info?
> 
> I had a look around and couldn't find any examples of this. What callsites
> are you concerned about?

I haven't looked in detail, but when debugging, it /looked/ like what the token function was getting was already initialized, but I can be wrong.
Note that PK11_GetTokenInfo does this:

/*
 * some buggy drivers do not fill the buffer completely, 
 * erase the buffer first
 */
PORT_Memset(info->label,' ',sizeof(info->label));
PORT_Memset(info->manufacturerID,' ',sizeof(info->manufacturerID));
PORT_Memset(info->model,' ',sizeof(info->model));
PORT_Memset(info->serialNumber,' ',sizeof(info->serialNumber));

So in fact, the pkcs module is not even supposed to fill with nulls, but with spaces (which matches what PK11_MakeString looks for, actually)

Man, this API is so broken.
I only landed the session part.
Keywords: leave-open
The string fields need to be padded with spaces, according to what
PK11_MakeString does to find the end of the string.

While here, factor all the string manipulations in the test module and
use some C++ template magic to do the right thing.

This changes the static asserts from (with clang):

pkcs11testmodule.cpp:45:3: error: static_assert failed
      "TestManufacturerID too long - make it shorter"
  static_assert(sizeof(TestManufacturerID) <= sizeof(pInfo->manufacturerID),
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

to:

pkcs11testmodule.cpp:46:3: error: static_assert failed
      "DestSize >= SrcSize - 1"
  static_assert(DestSize >= SrcSize - 1, "DestSize >= SrcSize - 1");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~
pkcs11testmodule.cpp:58:3: note: in instantiation of function
      template specialization 'CopyString<32, 63>' requested here
  CopyString(pInfo->manufacturerID, TestManufacturerID);
  ^

which actually gives more information than before: it gives the length of
both buffers.
Attachment #8563225 - Flags: review?(dkeeler)
Comment on attachment 8563225 [details] [diff] [review]
Properly initialize string fields from the PKCS#11 test module

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

LGTM - thanks!
Attachment #8563225 - Flags: review?(dkeeler) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/bd34e4330662
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.