1012855 - Negatus: Files are being written with corrupt data.

Status: RESOLVED FIXED

(Testing Graveyard :: SUTAgent, defect)

Description

[Syd Polk :sydpolk]

Negatus writes past the end of a buffer. On Linux desktop, this is mostly benign, as apparently, the allocator leaves unused space. On the Mac, however, this causes Negatus to write out corrupted files when receiving files from a server.

Comment 1

[Syd Polk :sydpolk]

Attachment: 0001-Bug-1012855-Files-are-being-written-with-corrupt-dat.patch

Stray write of a '\0' one byte past the end of a buffer. There should be no need to write out any null terminators since this package deals with data, not strings.

Comment 2

[Mark Côté [:mcote]]

Comment on attachment 8425054 [details] [diff] [review]
0001-Bug-1012855-Files-are-being-written-with-corrupt-dat.patch

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

Thanks for catching it!  I addressed your code comments below; feel free to change as appropriate and then push to the main repo.

::: src/CommandEventHandler.cpp
@@ +313,1 @@
>    getcwd(buffer, BUFSIZE);

No, that's why we pass BUFSIZE in.  According to the man page, "If the length of the absolute pathname of the  current  working  directory,  including the terminating null byte, exceeds size bytes, NULL is returned, and errno is set to ERANGE".

Technically we should be checking for an error here, admittedly, but at least it won't randomly corrupt memory.

::: src/Shell.cpp
@@ +25,4 @@
>        continue;
>  
>      fgets(buffer, BUFSIZE, iface);
> +      // This assumes that the last character is a newline. If the line is longer than BUFSIZE, that won't be the case. Benign.

True.  Probably another case for error checking, even if it's just to immediately fail, since the chances of that line being longer than BUFSIZE should be approximately zero.

Comment 3

[Mark Côté [:mcote]]

https://github.com/mozilla/Negatus/commit/7ea107ca476efb56fa274c207224710af18ae6e7