We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 122

Summary: SDL_RWops bug fixes
Product: SDL Reporter: Sam Lantinga <slouken>
Component: fileAssignee: Andreas Schiffler <aschiffler>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: aschiffler
Version: 2.0.0   
Hardware: All   
OS: All   
Attachments: sdl_rwops.changes
sdl_rwops.diff
Make use of the unused type member

Description Sam Lantinga 2006-01-31 10:10:44 UTC 
Date: Tue, 24 May 2005 00:43:05 -0400
From: Antonio SJ Musumeci <asm3072@njit.edu>
To: sdl@libsdl.org
Subject: [SDL] SDL_RWops bug fixes
-- Message: 16002   --  Next: 16005   ----------------------------------------
This code with SDL-1.2.8 and CVS:

#include <stdio.h>                
#include <stdlib.h>  
#include <SDL.h>        

int
main(int argc, char** argv)        
{
   char alphabet[26] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
   char buffer_file[27] = {0};
   char buffer_mem[27]  = {0};
   int rv_file;
   int rv_mem;
   FILE* file_ptr;
   SDL_RWops* rwop_file;      
   SDL_RWops* rwop_mem;

   file_ptr = fopen("./blah", "w");
   fwrite(alphabet, 1, sizeof(alphabet), file_ptr);
   fclose(file_ptr);

   rwop_mem  = SDL_RWFromMem(alphabet, sizeof(alphabet));
   rwop_file = SDL_RWFromFile("./blah", "r");
   rv_mem  = SDL_RWread(rwop_mem , buffer_mem,  5, 6);
   rv_file = SDL_RWread(rwop_file, buffer_file, 5, 6);
   printf("From File: %d %s\n"
          "From Mem:  %d %s\n",
         rv_file,
         buffer_file,
         rv_mem,              
         buffer_mem);  
   printf("Seek end of File: %d\n"
         "Seek end of Mem:  %d\n", 
         SDL_RWseek(rwop_file, 0, SEEK_END),       
         SDL_RWseek(rwop_mem , 0, SEEK_END));
   SDL_RWclose(rwop_file);
   SDL_RWclose(rwop_mem);

   return 0;
}


Produces this output:

 From File: 5 ABCDEFGHIJKLMNOPQRSTUVWXYZ
 From Mem:  5 ABCDEFGHIJKLMNOPQRSTUVWXY      
Seek end of File: 26      
Seek end of Mem:  26     

Attached is a patch which fixes this along with a few other
inconsistencies and bugs in SDL_rwops.c... it also enhances the API. It
should however still be compatible. Also attached is a list of all the
changes/enhancements made by this patch.

-Antonio SJ Musumeci
Comment 1 Sam Lantinga 2006-01-31 10:11:02 UTC 
Created attachment 57 [details]
sdl_rwops.changes
Comment 2 Sam Lantinga 2006-01-31 10:12:44 UTC 
Created attachment 58 [details]
sdl_rwops.diff

This patch mostly looks okay, but the API changes make it a good candidate for 1.3.

If you apply it, make sure you take out the part that removes the "unknown" portion of the union, which is used by custom rwops.
Comment 3 Ryan C. Gordon 2006-01-31 10:35:15 UTC 
The bug fixes in that patch are already in CVS, but the rest got rejected as an API change (but he DOES make some good points...the RWops interface could stand to be enhanced for 1.3!).

--ryan.
Comment 4 Sam Lantinga 2006-01-31 11:30:37 UTC 
(In reply to comment #3)
> The bug fixes in that patch are already in CVS, but the rest got rejected as an
> API change (but he DOES make some good points...the RWops interface could stand
> to be enhanced for 1.3!).

Indeed!  Okay, thanks, we'll keep this around for review/inclusion in 1.3
Comment 5 celtic.minstrel.ca 2010-10-09 09:17:11 UTC 
Created attachment 539 [details]
Make use of the unused type member

Instead of removing the unused type member, why not make use of it? At present there is no way to tell what kind of stream is used by the SDL_RWops, and the type member seems like it is there specifically for that purpose. While determining the type of a stream is not of huge importance, I think it could be useful for people extending the RWops interface, and it's a trivial change to make.

The attached patch is against revision 4890, if that's important.
Comment 6 Andreas Schiffler 2013-03-13 01:12:24 UTC 
Assigning to myself to review changes and apply fixes/suggestions.

Plan:
- bugs will be reviewed and fixes ported/applied
- test coverage will be added where applicable/possible
- any API changes will be rejected since the SDL 2.0 ABI has been locked
- the "unused type member" update will be included in a slightly modified form
Comment 7 Andreas Schiffler 2013-03-13 10:48:14 UTC 
Bug status SDL 2.0 HG:
   -Reading from memory was not consistant with how reading from files worked.
   A file of 55 bytes stored into 'ptr' by using 6 sets of 10 bytes would copy
   55 bytes into 'ptr' and SDL_RWread would return 5. But the same act done on
   mem RWop would result in 50 bytes copied and 5 returned.
Added test coverage. 
No repro on SDL 2.0.

  -Seems that SDL_RWclose should return fclose's return value instead of 0
-1 return is by design and documented.
Added test coverage for return value of SDL_RWclose.

  -Since there is no feof or ferror wrappers... if read or write return less 
   than requested... it was not known why. Added SDL_RWeof, SDL_RWerror, and
   SDL_RWclearerr
Won't fix since ABI change.

  -mem_writeconst() would return a -1... but fwrite nor mem_write would return
   a -1 on an error... especially since the concept of knowing an error occured
   didnt exist... return 0 now and sets flags as error.
No repro.

  -since it is obvious RWops is based off stdio FILE's... it would make sense
   to use the same data types. 
    > ftell returns a long... RWseek returns ftell so RWseek returns long
    > seek's offset argument is now a long
    > read and write take size_t's instead of int's and return size_t's
Already matches SDL2.0's implementation. 
   
  -where RWop was free'd in RWclose functions... appears it should have been
   using SDL_FreeRW since RWFromX's use SDL_AllocRW
No repro. CR'ed that all _close functions call SDL_FreeRW.

  -In unix_to_mac function... called from SDL_RWFromFile with a 
   const char* file = NULL would try to strlen(NULL)
No repro. Function does not exist anymore.

  -No check on the malloc used in unix_to_mac... put in check.
No repro. Function does not exist anymore.

  -mem_read and mem_write didnt check for overflows the same way fread/fwrite
   do (at least how dietlibc impliments them)... if num or size are 0
   return 0... if ((num * size) / num) != size) return 0

Misc:
  -Fixed some comments in SDL_rwops.h
Skipped.

  -renamed a few arguments for consistancy
Skipped.

  -SDL_RWops.type is completely unused... removed.
Updated to use type.

  -SDL_RWops.unknown also unused... removed.
Won't fix since ABI change.

  -Added SDL_OutOfMemory error setting to any malloc's
No repro. CR'ed that all mallocs set this error.

  -unknown values for 'whence' now also set RWerror
No repro. CR'ed that error is being set. Added test coverage.

Enhancements:
  -Added the concept of SDL_RWeof, SDL_RWerror, and SDL_RWclearerr
Won't fix since ABI change.

  -Created SDL_RWFromMallocedMem(void* ptr, size_t size)
   If 'ptr' is NULL... it allocates 'size' bytes for you, otherwise
   'ptr' must be from standard malloc. If you attempt to write past the
   end of the buffer it will realloc the buffer. You may also treat it
   like a file stream works... you can seek past the actual end and
   when you attempt to write data it will realloc and copy the data. Area
   between the original end and the seeked position will contain '\0''s
   just as a write performed on a file which was seeked past it's end.
   If realloc fails the buffer stays the same and the error flag is set.
Skipped. ABI change.

   - Make use of the unused type member.
Added defines and setting of type field.
Added test coverage.
Comment 8 Andreas Schiffler 2013-03-13 11:37:35 UTC 
Fix checked in.
http://hg.libsdl.org/SDL/rev/681820ca0e78

> ./testautomation --filter RWops
...
INFO:  03/13/13 16:36:36: Run Summary: Total=10 Passed=10 Failed=0 Skipped=0