strstr is still faulty since reported in 2002?

Go To Last Post
22 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Whats with the strstr() function.

I had all my strings in standard notation like this …

Char word_simnum[] = "";

The strsrt function worked OK for characters, but number characters would return a pointer for a partial match. A bug .. ?

I then started running out of RAM and decided to put the strings into PROGMEM.

const char word_simnum[] PROGMEM = “”;
const char word_portnum[] PROGMEM = “
”;

PGM_P text_strings[] PROGMEM = {
word_simnum,
word_portnum }

Then in a routine, I copy the string from Program memory into an array ..

strcpy_P(string_data,pgm_read_word(&(text_strings[0])));

and then search data received via a modem connected on a serial ports.
Now I if 'simnum' is being searchec for and is found, great...it works BUT
the 'num' part of portnum also returns a search 'hit' This is clearly wrong.

This issue has been noted here in 2002.

https://www.avrfreaks.net/index.p...

Anybody got any ideas other than writing my own string search routine?

Thanx
Brian

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am not clear that you get the right text into your string_data[] buffer.

The strstr() function is pretty simple, and I cannot believe that it is wrong.

You imply that strstr("abcdef", "cde") works
but strstr("123456", "34") does not
or strstr("123456", "78") returns !(NULL)

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Let me make it easy to see...

strstr("1234") ... works
strstr("1234 ") also returns a valid pointer ..it is seeing the 'num' part as a match and 'port' part has no effect .

Remember this only falls over when using coping strings out of PROGMEM. Normal strings declared within functions worrk OK ...except for number characters which do not.

And yes .. as simple as the strstr is, it does NOT work properly. The only thing I stand corrected on it that documentation refer to const strings. Don't know if this has anything to do with it.

Just tried the strstr_P function ... same results.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
strstr("1234") 

Is not valid. I only see three " in that - you need four for it to be valid.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson ... you have lost me ... don't follow.
The expression you posted will return a positive hit .. which it should not.

Here is another link stating the problem, but it was meant to be fixed. Could this problem still be an issue?

http://www.egnite.de/pipermail/e...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
strstr("1234")

What is the ACTUAL content of your parameters ?

Obviously your quoted strings could never match. strstr() makes no attempt to use pattern matching. Or at least it should not.

Please quote a proper example.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> This issue has been noted here in 2002.

Which is nothing like a formal bug report, and consequently epxerienced
what it was bound to experience anyway: it got lost, because nobody who is
really in charge for the code knew about it.

If you've got a reproducible test case (but please *not* a 200 KiB
ZIP archive, but a stripped-down minimal one), then please file a
bug report for the avr-libc project at

https://savannah.nongnu.org/bugs...

> Remember this only falls over when using coping strings out of PROGMEM.
> Normal strings declared within functions worrk OK.

This statement, however, causes me to suspect your environment. After
all, how should the called strstr() get *any* information about whether
the string has been copied from progmem, or might have been assembled in
RAM by the caller? This rather smells like some kind of resource
exhaustion (clobbered RAM or such).

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I would guess that we are not passing string addresses but some other data.

We will wait for a "real" example with real parameters.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

For the sake of getting code to a customer, I have ditched the PROGMEM idea and using strings declared locally. This at least gives me a working system. When I have some time, I will post more detail here.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I just looked at the source code in 1.4.6 and it does appear to be buggy. I can provide a working replacement if interested?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hmmm, after closer examination it appears to be okay.

Shouldn't your line:
strcpy_P(string_data,pgm_read_word(&(text_strings[0])));

be:
strcpy_P(string_data,&text_strings[0]);

Never mind, didn't notice you were storing the pointers in flash as well. Oh well, tomorrows another day another world :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dear DIY,

Apologies are due. I have looked at strstr.s in avr-libc-1.4.5 and it is WRONG.

Re-writing the assembler code as C shows the problem.

#include 

char *strstr(const char *str1, const char *str2)
{
	char chr1, chr2, *X = str1, *Z = str2;
	if ((chr2 = *Z++) == 0) return str1;
	while (1) {             // .findstart:
		X = str1;	// INSERTING this line makes it better .kbv
		Z = str2;
		chr2 = *Z++;
		do {            // .findstart_loop:
			if ((chr1 = *X++) == 0) return (char *)0;
			str1 = X;
		} while (chr1 != chr2);
		while (1) {     // .stringloop:
			if ((chr2 = *Z++) == 0) return str1 - 1;
			if ((chr1 = *X) == 0) return (char *)0;
			if (chr1 != chr2)
				break;
			X++;
		}
	}
}

#ifdef __TURBOC__
#include 

#define D(x,y)	printf("strstr(\"%s\",\"%s\"):\"%s\"\n", x, y, strstr(x, y)); 
void main(void)
{
	D("abcdef", "");
	D("", "cde");
	D("abcdef", "cde");
	D("abcdef", "xyz");
	D("000123", "01");
	D("000123", "001");
	D("000123", "789");
}
#endif

in the assembler source, the line

X_movw XL, s1_lo

needs to be moved to just after the .L_findstart: label

Looking at my "converted" C source, you can see that the C statement "char *X = str1" is what you are moving.

I assume that the same logic is involved in the strstr_P() variant.

I believe that someone has volunteered to do the bug report.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Please file a bug report (to avr-libc), otherwise your findings will get lost
again.

(Thanks for analyzing it!)

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Looks fixed in 1.4.6, i.e., the outer loop starts like this:

0:	X_movw	XL, s1_lo

/Lars

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
Looks fixed in 1.4.6, i.e., the outer loop starts like this:

Yeah, I had the versions mixed up myself... had me confused for a bit there :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Seems some progress is being made here.

But I am a little confused ... as the current Doc version that came with last WinAVR I downloaded say 1.4.6. This is version 20070525.

I have just downloaded avr-libc on its own and I see there are differences to what I have installed via WinAVR. Something smells funny here?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

So what do I do now ... how do I check to see what I have installed. Can't find any of the above code in any files to check what I have. How do I progress?

Thanx
Brian

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As far as I can see.

You observed an error with strstr()
It is confirmed that v1.4.5 and earlier are WRONG
The current v1.4.6 source code is correct. The edit history seems to be March 2007.

You said that strstr() succeeded with an incorrect match. The error that I found failed on some good matches.

I suggest that you pass my examples through your strstr() and report on the result. I think that you may be passing corrupted parameters.

Try with my C code. Then comment out the new statement, and see if it replicates.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

BTW, Joerg and I are very grateful that more people are taking a look at the avr-libc source code. We're very happy to get any problems fixed. Just please let us know of any problems through the right channels, like the avr-libc Bug Tracker. We get an immediate email any time new bugs are posted, or comments to bugs are added. This is a lot faster than it takes for us to come wander over to AVR Freaks and look at the postings here. :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

dear DIY,

I am not sure how you get at the version number of your libc.a

I can only suggest that you do a

avr-ar tv libc.a | egrep strstr

and see when the library module binary was built. If it is before about May 2007 then it is probably the old version. I have tried my own "ident" and get no version strings out of the archive.

Probably easier to just try the examples to a debugger or a UART.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I come back here with my tail buried beneath my legs. I am quite embarrassed. :oops: I have found my problem. As a result of a hectic time schedule and 3 solids weeks of 6:30am till 11:30pm every day, I am starting to make mistakes. And the error was a simple one which I simply did not see after converting and editing my code to use strstr_P

My code looks like this

str_ptr = strstr_P(xml,pgm_read_word(&(text_strings[24])));
if(str_ptr) {

str_ptr += 9;
EEPROM_write(0x1A, str_ptr[0]);
EEPROM_write(0x1B, str_ptr[1]);
EEPROM_write(0x1C, str_ptr[2]); }

I have a whole lot of these expressions, but one had this ….

// updata Ethernet chip MAC Address

str_ptr = strstr_P(xml, pgm_read_word(&(text_strings[9])));
if(strstr) {

str_ptr += 10;
convert_to_hex(str_ptr,hex_array,6,1);
hex_array[6] = 0x3C;
update_eeprom(0x80,hex_array); }

Spot the error … Yes .. a bad mistake. Just too many late nights. I humble apologize to all for waiting your time. A bit surprised the compiler did not throw and error on it though.

After seeing errors being reported about strstr, I was convinced I was onto something... which David has identified.

This all said though, there is still an issue with numbers. I will look into this a little further before shooting my mouth off again though.

Brian

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> A bit surprised the compiler did not throw and error on it though.

Testing a pointer inside a conditional is not an error. Actually,
it's even common practice to test ordinary (object) pointers
that way, e. g. using something like:

void foo(int a, int *resp)
{
   ...
   if (resp) *resp = result;
}

That way, the caller can pass the address of a local variable
down into foo(), or it can simply pass NULL if he's not interested
in that particular result. Similar could apply for a function
pointer:

void
foo(int x, int (*initfunc)(void))
{
  ...
  if (initfunc) result = initfunc();
  ...
}

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.