[Bug 14788] New: Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Summary: Check return codes everywhere
           Product: Wine
           Version: CVS/GIT
          Platform: All
        OS/Version: All
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: -unknown
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Some source files contain unchecked function calls.

Examples:
- malloc
  => main

http://source.winehq.org/git/wine.git/?a=blob;f=tools/fnt2fon.c;h=aa7d72b84b...

=> build_new_path

http://source.winehq.org/git/wine.git/?a=blob;f=loader/freebsd.c;h=0d0eb5767...

=> pdb_jg_read

http://source.winehq.org/git/wine.git/?a=blob;f=tools/winedump/pdb.c;h=1d9fc...

- strdup
  => build

http://source.winehq.org/git/wine.git/?a=blob;f=tools/winegcc/winegcc.c;h=11...

- fprintf
  => writeHeader

http://source.winehq.org/git/wine.git/?a=blob;f=dlls/wineps.drv/afm2c.c;h=3f...

- fputs
  => REGPROC_export_string

http://source.winehq.org/git/wine.git?a=blob;f=programs/regedit/regproc.c;h=...

- pthread_mutex_lock, pthread_mutex_unlock
  => __pthread_atfork

http://source.winehq.org/git/wine.git/?a=blob;f=loader/kthread.c;h=cc51cc060...

Would you like to add more error handling for corresponding return values?

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #1 from Vitaliy Margolen   2008-08-07 12:07:33 ---
Send patches to wine-patches mailing list.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #2 from Markus Elfring   2008-08-07 13:01:55 ---
Would you like to detect every error situation as early as possible?

I am unsure which approaches will be acceptable in your software to complete
error detection and handling.
- How do you think about the reaction "exit(errno)" or "abort()"?

- Would you like to reduce the efforts for error code checking by an exception
class hierarchy?
http://dietmar-kuehl.de/mirror/c++-faq/exceptions.html#faq-17.1
http://cexcept.sourceforge.net/

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #3 from Vitaliy Margolen   2008-08-07 15:22:24 ---
Move this discussion to wine-devel.
Some stuff you pointed out is valid, some - irrelevant. It makes no sense
checking all the output in secondary utilities. It will make their code more
complicated for no good reason.

Also it seems you totally don't understand what Wine is, and how much of c++ it
has...

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Juan Lang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #4 from Juan Lang   2008-08-07 21:12:47 ---
I'm marking this invalid.  Several times memory checking, leaking, etc. within
the utilities used to build wine have been brought up, and each time Alexandre
says it's not worth the effort.  We don't have many "tracker" bugs here, so
discussing possible enhancements throughout the codebase doesn't seem like a
good use of bugzilla.  Besides this, most of the issues you brought up are not
recoverable errors anyway, so there appears to be no benefit of checking the
return values in most of the instances you point out.  If you'd like to fix a
particular instance, either send a patch to wine-patches, or discuss it on
wine-devel.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Dmitry Timoshkov   2008-08-08 03:54:42 ---
Closing.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #6 from Markus Elfring   2008-08-08 06:21:57 ---
I know that it is hard to achieve consensus on details for approaches to do
"the right thing". I try to get acceptance for potential updates before I would
fiddle with source code.

Would you like to look at opinions from the discussion
"PTHREAD_MUTEX_INITIALIZER"?
http://groups.google.de/group/comp.programming.threads/browse_frm/thread/1d3...

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #7 from Dmitry Timoshkov   2008-08-08 07:28:14 ---
As Vitaliy said feel free to send the patches to wine-patches, and start
the discussion at wine-devel.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #8 from Markus Elfring   2008-09-13 03:43:33 ---
Created an attachment (id=16068)
 --> (http://bugs.winehq.org/attachment.cgi?id=16068)
update suggestion

Would you like to integrate any of the proposed changes for improved software
stability?

How do you think about to look for any remaining issues by static source code
analysis?
http://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis#C

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #16068|text/plain                  |application/octet-stream
          mime type|                            |
  Attachment #16068|1                           |0
           is patch|                            |

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #9 from Vitaliy Margolen   2008-09-13 11:40:45 ---
I appreciate your effort, however most changes are invalid. You can't just
"exit(errno)" when fclose() failed! This is as harmless as it gets. Yet you
just crash the whole application because of it.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #10 from Markus Elfring   2008-09-14 02:21:11 ---
Which reaction do you like more for failed (f)close function calls?

http://en.wikipedia.org/wiki/Crash-only_software

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #11 from Dmitry Timoshkov   2008-09-14 02:31:37 ---
(In reply to comment #10)
> Which reaction do you like more for failed (f)close function calls?

No, handling fclose() failures is absolutely useless.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #12 from Markus Elfring   2008-09-14 02:51:02 ---
I have got the opinion that such destruction or resource release failures
should not be ignored.
Will it make sense to adjust the reactions by distinguishing the error
indication for file descriptors that became invalid and interruption of file
operations?

http://opengroup.org/onlinepubs/009695399/functions/close.html
http://opengroup.org/onlinepubs/009695399/functions/fclose.html

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #13 from Dmitry Timoshkov   2008-09-14 03:01:56 ---
(In reply to comment #12)
> I have got the opinion that such destruction or resource release failures
> should not be ignored.
> Will it make sense to adjust the reactions by distinguishing the error
> indication for file descriptors that became invalid and interruption of file
> operations?
> http://opengroup.org/onlinepubs/009695399/functions/close.html
> http://opengroup.org/onlinepubs/009695399/functions/fclose.html

These worries are theoretical. If you find out that some resource (you are
relying upon) abruptly becomes invalid that would mean a bug in the underlying
OS. In any case what would handling such a case buy us? Absolutely nothing.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #14 from Dmitry Timoshkov   2008-09-14 03:04:17 ---
Besides, have you looked at the example code provided in the links you
posted?

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #15 from Markus Elfring   2008-09-14 03:19:31 ---
Yes - I know it is common to be "sloppy" with error checking in such short
demonstration code.
I prefer to detect every error situation in the source code as soon as possible
to improve software robustness and correctness.

Would you like to care for (unexpected) memory corruption?

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #16 from Dmitry Timoshkov   2008-09-14 03:29:31 ---
(In reply to comment #15)
> Would you like to care for (unexpected) memory corruption?

If that's again a theoretical speculation - no. If you know an exact
problem - open a bug report.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #17 from Markus Elfring   2008-09-14 03:50:56 ---
Memory corruption is not only a theoretical issue.
http://en.wikipedia.org/wiki/Buffer_overflow
http://en.wikipedia.org/wiki/ECC_memory#Error-correcting_memory

I recommend to achieve complete error detection. I do not like ignorance of
(unpleasant) return values.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|patch                       |
         OS/Version|All                         |other
           Platform|All                         |Other
            Version|CVS/GIT                     |unspecified

--- Comment #18 from Dmitry Timoshkov   2008-09-14 04:01:57 ---
(In reply to comment #17)
> Memory corruption is not only a theoretical issue.
> http://en.wikipedia.org/wiki/Buffer_overflow
> http://en.wikipedia.org/wiki/ECC_memory#Error-correcting_memory
> I recommend to achieve complete error detection.

Looks like you don't have any problems in Wine to point out.
> I do not like ignorance of
> (unpleasant) return values.

As was said several times already not all such cases need an action,
at least so far you haven't demonstrated anything useful to worry about.

I'd prefer to stop debates which have nothing to do with real bugs in Wine.
Again, If you know an exact problem in Wine code - open a bug report, stop
speculating about theoretical problems.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Markus Elfring  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         OS/Version|other                       |All
           Platform|Other                       |All
            Version|unspecified                 |CVS/GIT

--- Comment #19 from Markus Elfring   2008-09-14 04:18:11 ---
Would you like to consider any updates for remaining unchecked function calls?
- malloc, strdup
- (f)printf, fputs, fputc, ftruncate, unlink
- pthread_...

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         OS/Version|All                         |other
           Platform|All                         |Other

--- Comment #20 from Dmitry Timoshkov   2008-09-14 04:30:36 ---
Please stop posting i this bug, it's invalid and closed.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|CVS/GIT                     |unspecified

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #21 from Markus Elfring   2008-09-14 09:50:21 ---
How do you think about to identify update candidates by compiler-specific
extensions like the tag "__attribute__ ((__warn_unused_result__))"?
http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html

Use cases:
(f)read, (f)write, ftell, lseek ...

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #22 from Vitaliy Margolen   2008-09-14 10:59:18 ---
(In reply to comment #19)
> Would you like to consider any updates for remaining unchecked function calls?
> - malloc, strdup
> - (f)printf, fputs, fputc, ftruncate, unlink
> - pthread_...

As far as malloc, strdup - they should not be used in Wine itself:
http://wiki.winehq.org/ReplaceMalloc

For other function some places might benefit from proper error checking for
these functions. However as I indicate before, each error have to be handled
properly not just "exit(errno);".

Of course this is relevant to Wine itself and it's tools. Adding these checks
to tests is counterproductive.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #23 from Markus Elfring   2008-09-15 08:13:46 ---
(In reply to comment #22)
> For other function some places might benefit from proper error checking for
> these functions. However as I indicate before, each error have to be handled
> properly not just "exit(errno);".

Which function should be called instead?

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #24 from Dmitry Timoshkov   2008-09-15 08:47:07 ---
If you reread the comments you will find the answers.

Please stop posting in this bug, it's invalid and closed.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #25 from Markus Elfring   2008-09-19 12:14:18 ---
I guess that a well-known analysis tool will remind interested software
developers about related open issues by detailed reports.
http://scan.coverity.com/rung1.html => http://scan2.coverity.com:7479/

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Markus Elfring  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |UNCONFIRMED
         Resolution|INVALID                     |

--- Comment #26 from Markus Elfring   2009-03-24 03:57:13 ---
Did the desire to look at a topic like "CWE-389: Error Conditions, Return
Values, Status Codes" increase after an update for the issue "Fix a memory leak
(Coverity 904)" was applied?
http://cwe.mitre.org/data/definitions/389.html

Will more function implementations be completed for this aspect?
Example:
WCMD_strdupW
http://source.winehq.org/git/wine.git/?a=blob;f=programs/cmd/wcmdmain.c;h=c5...

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #27 from Vitaliy Margolen   2009-03-24 10:21:16 ---
Why did you reopened the bug? If you want to fix the return code check - send
patches.

DO NOT REOPEN.

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

Vitaliy Margolen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #28 from Vitaliy Margolen   2009-03-24 10:21:32 ---
Closing

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #29 from Markus Elfring   2009-03-24 13:22:43 ---
(In reply to comment #27)
> [...] - send patches.

My attachment #16068 contains suggestions for software changes. There are still
more ways to fix open issues like "CWE-690: Unchecked Return Value to NULL
Pointer Dereference" (and "CWE-252: Unchecked Return Value" in general).

Are there any chances to apply usual advices by tools from the software area
"aspect-oriented programming"?
http://research.msrg.utoronto.ca/ACC/Tutorial#A_Reusable_Aspect_for_Memory_A...

[Bug 14788] Check return codes everywhere

http://bugs.winehq.org/show_bug.cgi?id=14788

--- Comment #30 from Dmitry Timoshkov   2009-03-25 05:13:22 ---
This bug is INVALID, which means that there is no bug in Wine as implied here,
so no any further action required.