58038 - if out of memory, lock file can be spool file

Status: RESOLVED INVALID

(MailNews Core :: Movemail, defect)

Description

[(not reading, please use [e-mail] instead)]

from #56666:

"What if malloc's heap is exhausted transiently just as you append ".lock".  It's
unlikely, but scary.

+    nsCAutoString lockstr(spoolnameStr);
+    lockstr.Append(".lock");

We'll end up with lockstr naming the spool file, which might lead to very bad
things happening.

Cc'ing roc cuz I thought he made MakeUnique non-racey.

/be"

Comment 1

[Adam D. Moss]

Hmm, I thought that this would be a no-brainer
but as it happens, nsCAutoString's Assign() just
returns void.

How is one supposed to tell if it fails -- any clues?

I could manually go and check that the string I asked
to be appended actually got appended, but that seems
a bit hackish...

Comment 2

[Brendan Eich [:brendan]]

Cc'ing scc -- help us, obi-wan!

/be

Comment 3

[Scott Collins]

check the length before and after.  Perhaps pre-flighting this with a
|SetCapacity| could help reduce the chance of a race ... probably only needed if
you actually expect the final length of the name to be greater than the
stack-store size of an |nsCAutoString|.  Sorry about the lack of error returns,
it was that way when I got it (well, actually, it returned a reference to
itself), but it's on my radar.

Comment 4

[Adam D. Moss]

Attachment: patch for 'safe' string-appending

Comment 5

[Adam D. Moss]

The attached patch checks that the length of the resulting
string is equal to the sum of the the original string and
the appended string.  It does this for the three times that
mozilla's movemail constructs a critical string with .Append()

I'd appreciate a review and then I'll go jump through the
super-review hoops also.  Thanks.

Comment 6

[Scott Collins]

|nsString|s and friends already know their own length.  Prefer the constant time
member function |Length()| over the linear |PL_strlen|.  You probably want to
use |NS_LITERAL_CSTRING| to define the parts, e.g.,

  #define LOCK_SUFFIX NS_LITERAL_CSTRING(".lock")

or else, for even less work, use |NS_NAMED_LITERAL_CSTRING| at the top of the
function that uses the define more than once

  NS_NAMED_LITERAL_CSTRING(lock_suffix, ".lock");

Using this scheme, the lengths are calculated at compile time, the appends are
faster, and you can say, e.g.,  |LOCK_SUFFIX.Length()|, or
|lock_suffix.Length()|, respectively, and there is not character copying until
the actual append.  In fact, if the file function that took your new name took a
|const nsAReadableCString&|, you wouldn't even have to worry about the
allocation problem and testing the lengths, because you could just pass it the
concatenation of the two name fragments, which produces a readable string with
no allocation or copying and can't fail for low memory, e.g.,

  some_file_operation(fileNameStr+lock_suffix);

Comment 7

[timeless]

how's this going, and will we be using nsfilespec or nsifile?

Comment 8

[(not reading, please use [e-mail] instead)]

as you can see, no activity on this bug for a while.

eventually, we'll move to nsIFile / nsILocalFile.

Comment 9

[Adam D. Moss]

I hope to be revisiting this issue soon.

Comment 10

[Adam D. Moss]

Futuring, helpwanted, NEW, movemail component.

Comment 11

[Adam D. Moss]

-> movemail owner

Comment 12

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 13

[:aceman]

Is this still relevant?
pkwarren has de-assigned himself in 2005, the 2008 assigning back to him is probably a mistake.

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #13)
> Is this still relevant?

I would think so.  joshua touched around this area in bug 384774, but afaics the precise lines of the patch in this bug are completely untouched.

Comment 15

[:aceman]

I can probably update this to use the .Length() system.

Comment 16

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #13)
> Is this still relevant?
> pkwarren has de-assigned himself in 2005, the 2008 assigning back to him is
> probably a mistake.

Strings are infallible by default, so if we can't allocate room for the longer string, we abort the application instead.

Comment 17

[:aceman]

Great.